Dates.Time compatibility with StepRangeLen by adding converting constructors

[mtd321]

This is improve the usability of Time by satisfying the expectation that Time(t::Time) == t, similar to other subtypes of TimeType and Period, and most other types.

For example, it resolves this unexpected behavior:

julia> using Dates

julia> ts = range(Time(0), step=Hour(1), length=23);

julia> ts[1]
ERROR: MethodError: no method matching Int64(::Time)
The type `Int64` exists, but no method is defined for this combination of argument types when trying to construct it.

[LilithHafner]

This seems quite sensible. Looking at the three concrete subtypes of DateTime:

For Date, the conversion is strange; semantically it should always be the zero time, I suppose, but the convert method throws which I like. This PR adds a layer of indirection to that error.

For DateTime, this PR doesn't change behavior.

For Time, Time(t::Time) should be a no-op and this PR changes it from a method error to a no-op.

---

From a documentation perspective, the Time(dt::DateTime) method is semantically nontrivial and has a good docstring. I like it how it is, but if this PR were to change the type signature, the docstring would have to change, too.

---

Perhaps it would be cleaner and semantically equivalent on existing concrete types to implement this as Time(t::Time) = t with no docstring on that new method?

[mtd321]

thank-you for the review and sending me back to look harder. My initial solution was based on Time <: TimeType and associated conversions, but I now consider that type relation to be unsound¹.

The docstring and the method signature should agree regarding parameter type, though this hasn't been done for the other conversion constructors - method Date(dt::TimeType) is documented as Date(dt:DateTime) and similarly for DateTime. It would be nice to address those discrepancies as part of this change too. I suspect they were made like that to work around the same issue - the single param fallback constructors that the first parameter to Int64 play badly with other expectations, in particular the StepRangeLen implementation expects to construct the container element from the reference element, even if it's the same type.

I accept your suggestion that it's better to simply define Time(t::Time) = t. I will apply the same solution to the other concrete types. (I'm calling it a copy constructor.) I'll re-title this PR.

Footnotes

1. It seems that original intention behind TimeType was to share calendrical methods between Date and DateTime - eg. dayofweek, monthday. When called with an instance of Time, such methods fail with no method defined days, so in that sense, instances of Time do not belong. Likewise, a generic method isless(x::TimeType, y::TimeType) works by promoting for Date and DateTime (so we can make the questionable assertion that a date/time is later than the same date - perhaps a lawyer would disagree!) but no such promotion involving Time. Looking to the documentation: TimeType types wrap Instant machine instances to provide human representations of the machine instant. Time, DateTime and Date are subtypes of TimeType. Machine instants form a monotonic sequence, and the instants could be represented as dates or date/times, whereas Time, which is really "time of day", is periodic (cyclic), and cannot properly represent a machine instant. ↩

[mtd321]

sorry - I seem to have gotten some other changes intermingled with my PR... not sure what to do about that.

[mtd321]

Just to highlight the periodicity this achieves:

julia> using Dates

julia> collect(range(Time(0), step = Hour(9), length=5))
5-element Vector{Time}:
 00:00:00
 09:00:00
 18:00:00
 03:00:00
 12:00:00

[LilithHafner]

sorry - I seem to have gotten some other changes intermingled with my PR... not sure what to do about that.

You could try git rebase -i master or you could do git reset master and then re-apply the changes you intend to make as a new commit (which you could get with git cherry-pick or just recreate manually). If all else fails it is certianly acceptable to make a new PR and close this one when the git history is messed up. Just leave a comment on both PRs to indicate the switchover.

[mtd321]

sorry - I seem to have gotten some other changes intermingled with my PR... not sure what to do about that.

resolved by force pushing onto my fork.

[mtd321]

I see there are build errors re documentation along the lines discussed.... i'll investigate.

@ Documenter /cache/build/builder-amdci4-5/julialang/julia-master/deps/jlutilities/depot/packages/Documenter/iRt2s/src/utilities/utilities.jl:47
┌ Error: no docs found for 'Dates.Date(::Dates.TimeType)' in `@docs` block in src/stdlib/Dates.md:706-731

[LilithHafner]

This PR has grown quite a bit since its original form. I'm going to try to find someone more familiar with the Dates library to take over reviewing.

[mtd321]

This PR has grown quite a bit since its original form. I'm going to try to find someone more familiar with the Dates library to take over reviewing.

much appreciated thank-you.

[LilithHafner]

I wasn't able to find another reviewer, so I'll continue to review this.

This PR contains a few distinct and separable changes.

1. Mentioning Time more prominently in the prose documentation. This is a good change that I would be happy to merge.
2. Refactoring some conversions, which I have yet to give a detailed technical review
3. Enabling cyclic Time ranges which is an expansion of the public API and needs broader review. It also has interesting consequences with respect to the assumptions we can make about ranges.

I'm tagging triage to discuss 3. We meet every other week on Thursdays or Fridays depending on time zone, with our next meeting on the 17th, 1:15 EDT or the 18th, at 2:15AM YAKT on google meet.

[LilithHafner]

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.

julia> r = range(Time(0), step = Hour(9), length = 5)
Time(0):Hour(9):Time(12)

julia> extrema(r)
(Time(0), Time(12))

[mtd321]

Ok that's nasty odd, but it looks to me like faulty short-cutting minimum and maximum methods for StepRangeLen. We could fix those to actually iterate the sequence. For comparison, this gives the expected result:

julia> extrema(identity, r)
(Time(0), Time(18))

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 its repr 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:

julia> length(eval(Meta.parse(repr(r)))), length(r)
(2, 5)

We could fix this too - there is already special handing in the show method to show ranges such StepRangeLen(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, or StrepRangeLen (or the types OrdinalRange, StepRange ) firmly specifies monotonicity. Extracting key references:

• range - ... a specialized array with evenly spaced elements and optimized storage
• AbstractRange - 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 oneunit
• StepRange - 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:

julia> using Mods

julia> r2 = range(Mod{24}(0), step=9, length=5)
Mod{24}(0):9:Mod{24}(12)

julia> collect(r2)
5-element Vector{Mod{24}}:
  Mod{24}(0)
  Mod{24}(9)
 Mod{24}(18)
  Mod{24}(3)
 Mod{24}(12)

julia> maximum(r2)
ERROR: MethodError: no method matching isless(::Mod{24}, ::Mod{24})

It already works because, unlike Dates.Time, Mods.Mod already has a copy constructor:

julia> Mod{24}(Mod{24}(0))
Mod{24}(0)

Now, that expectation of a copy constructor for T in StepRangeLen (ie that T(t::T) = T) is the original problem that I solved by adding copy constructor for Time. But, in retrospect, I now think a better solution could be to relieve StepRangeLen 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):

--- a/base/range.jl
+++ b/base/range.jl
@@ -974,7 +974,7 @@ unsafe_getindex(v::OneTo{T}, i::Integer) where T = convert(T, i)
 unsafe_getindex(v::AbstractRange{T}, i::Integer) where T = convert(T, first(v) + (i - oneunit(i))*step_hp(v))
 function unsafe_getindex(r::StepRangeLen{T}, i::Integer) where T
     u = oftype(r.offset, i) - r.offset
-    T(r.ref + u*r.step)
+    convert(T, (r.ref + u*r.step))
 end

Sorry for the long reply, but I think it's worth digging into the details to help decide what is the best way forward.

[mtd321]

I wasn't able to find another reviewer, so I'll continue to review this.

This PR contains a few distinct and separable changes.

1. Mentioning Time more prominently in the prose documentation. This is a good change that I would be happy to merge.

I am happy to separate out a documentation change, but I'd like to first get a direction on the other changes.

2. Refactoring some conversions, which I have yet to give a detailed technical review

Please hold off on that till after triage. Really the only reason I was adding conversions was to overcome the issue using Time with StepRangeLen. As per my lengthy comment to the oddity you found with extrema, maybe there's a better way to address the issue.

3. Enabling cyclic Time ranges which is an expansion of the public API and needs broader review. It also has interesting consequences with respect to the assumptions we can make about ranges.

Indeed, although those concerns with ranges exist anyway and appear more fundamental. There are other ways of getting cyclic time sequences - eg:

julia> Time(0) .+ Hour.(range(0, step=9, length=5))
5-element Vector{Time}:
 00:00:00
 09:00:00
 18:00:00
 03:00:00
 12:00:00

I'm tagging triage to discuss 3. We meet every other week on Thursdays or Fridays depending on time zone, with our next meeting on the 17th, 1:15 EDT or the 18th, at 2:15AM YAKT on google meet.

I'd like to attend, and I'll try, but there's a good chance I'll be sleeping at 03:15 UTC+10:00, so I apologize in advance if I don't attend.

My goal is to improve the usability of julia - I appreciate the feeback you've given, which has highlighted that there are subtleties I wasn't initially aware of. Please let me know if there's a better way forward than PR, such as raising a github issue, or a new discussion on julia discourse.

(I'm going to rebase this PR, update the doco changes, and mark this is draft to reflect the true status.)

[LilithHafner]

Thank you for these comments!

[mbauman]

It's very wild that adding these converting constructors — which I think probably should exist in some fashion — has this impact on ranges.

This wrapping of ranges also happens with integers, but it's largely avoided in two ways:

• integer range() (and :) constructions make StepRanges, not StepRangeLens. Overflowing a StepRange spits back an empty range.
• the range arithmetic on small integers promotes to Int, which then cannot be constructed (or converted) back to the smaller int size (and thus they can't overflow).

So it's only really seen in the very explicit (and unwieldy):

julia> r = StepRangeLen(typemax(Int)-2, 1, 5)
9223372036854775805:1:-9223372036854775807

julia> collect(r)
5-element Vector{Int64}:
  9223372036854775805
  9223372036854775806
  9223372036854775807
 -9223372036854775808
 -9223372036854775807

julia> extrema(r)
(-9223372036854775807, 9223372036854775805)

Dates' Hour type would do exactly the same with much more common values.