-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Dates.Time compatibility with StepRangeLen by adding converting constructors #58846
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
mtd321
wants to merge
5
commits into
JuliaLang:master
Choose a base branch
from
mtd321:convert-Time-from-TimeType
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+116
−40
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4520c5a
Introduce method to convert to Time from TimeType
mtd321 abcb6c9
address feedback by dropping the generic conversion constructor in fa…
mtd321 b4fe696
fix Date Functions docs references; refresh doco for
mtd321 4e5679b
two sentences more clear
mtd321 1f4b093
better phrasing about periodic time
mtd321 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems... odd.
It violates
extrema(range) == minmax(first(range), last(range))
But the definition of
extrema
assumes this identity, and therefore gets the wrong answer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that's nasty odd, but it looks to me like faulty short-cutting
minimum
andmaximum
methods forStepRangeLen
. We could fix those to actually iterate the sequence. For comparison, this gives the expected result:I suppose another unhappy oddity that initially drew your attention is that the returned range violates the implied expectation (see
?show
- The representation used by show ... should be parseable Julia code when possible) that itsrepr
could be used to construct an equivalent range, but in this case it stops early (length 2) rather than wrapping around to the full length of 5:We could fix this too - there is already special handing in the
show
method to show ranges suchStepRangeLen(0, 0, 0)
with constructor syntax.Are ranges expected to be monotonic? Both the above oddities are due to that unsatisfied expectation. But nothing normative in the docs for
range
,AbstractRange
, orStrepRangeLen
(or the typesOrdinalRange
,StepRange
) firmly specifies monotonicity. Extracting key references:range
- ... a specialized array with evenly spaced elements and optimized storageAbstractRange
- Supertype for linear ranges ...StepRangeLen
- a range r where r[i] produces values of type T ...OrdinalRange
- ... with spacings of type S. The steps should be always-exact multiples of oneunitStepRange
- the step between each element is constant. Neither T nor S should be floating point types.If so, ideally that expectation should be codified somehow, or at least documented. Here is an example from another package that already behaves similarly to the proposed behavior for
Time
:It already works because, unlike
Dates.Time
,Mods.Mod
already has a copy constructor:Now, that expectation of a copy constructor for T in
StepRangeLen
(ie thatT(t::T) = T
) is the original problem that I solved by adding copy constructor forTime
. But, in retrospect, I now think a better solution could be to relieveStepRangeLen
of this undocumented expectation, by changing to use the available parametric self-conversion (and then document the expectation of a conversion from R to T where they are not identical):Sorry for the long reply, but I think it's worth digging into the details to help decide what is the best way forward.