Conversation
| trainingSchedule : SamplingSchedule, | ||
| validationSchedule : SamplingSchedule | ||
| ) : (SequentialModel, Int) { | ||
| mutable bestSoFar = Default<SequentialModel>() |
There was a problem hiding this comment.
This may be worth an extended discussion. Before deprecating Default, it was somewhat of an idiom to use Default<UdtType>() w/ ItemName <- foo to kind of get named-item initializers instead of relying purely on positional construction. It may be good to have a language feature similar to Rust's {}-style structs as an alternative way of creating UDT values.
|
|
||
| function ExampleModel() : ML.SequentialModel { | ||
| return Default<ML.SequentialModel>() | ||
| w/ Structure <- [ |
There was a problem hiding this comment.
Similarly here, I think the original code was more readable even if less correct... from that perspective, I agree with your change but would like to kick off a discussion on more readable alternatives going forward?
There was a problem hiding this comment.
I agree that the original code was more readable. Do you suggest to keep the version using Default?
There was a problem hiding this comment.
I think getting rid of Default is worth temporarily having less readable code, so no disagreement with your PR so much as that I want to keep momentum on a longer-term solution, I guess?
| mutable current = state; | ||
| mutable result = [Default<'State>(), size = Length(array)]; | ||
| // initialize with current, and then overwrite in loop | ||
| mutable result = [current, size = Length(array)]; |
There was a problem hiding this comment.
Does this need an early exit if array is empty, or is that implied by size = 0?
There was a problem hiding this comment.
That is implied by size = 0 since we do not need to access or compute some element based on the 0 index in this case.
There was a problem hiding this comment.
Makes sense, wasn't entirely sure from a readthrough so wanted to double-check. Thanks!
2 participants
Fixes #546 and removes all other occurrences of
Default; also removes two warnings.