Skip to content
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

Range indexing fails #78

Closed
iamed2 opened this issue Nov 18, 2019 · 6 comments · Fixed by #132
Closed

Range indexing fails #78

iamed2 opened this issue Nov 18, 2019 · 6 comments · Fixed by #132

Comments

@iamed2
Copy link
Member

iamed2 commented Nov 18, 2019

julia> using Dates, TimeZones, Intervals

julia> st = HE(ZonedDateTime(2014,1,1, tz"America/Winnipeg"))
AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2014, 1, 1, tz"America/Winnipeg"), Inclusivity(false, true))

julia> r = st:Hour(1):(st + Day(1))
AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2014, 1, 1, tz"America/Winnipeg"), Inclusivity(false, true)):1 hour:AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2014, 1, 2, tz"America/Winnipeg"), Inclusivity(false, true))

julia> collect(r)[1:2]
2-element Array{AnchoredInterval{-1 hour,ZonedDateTime},1}:
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2014, 1, 1, tz"America/Winnipeg"), Inclusivity(false, true))
 AnchoredInterval{-1 hour,ZonedDateTime}(ZonedDateTime(2014, 1, 1, 1, tz"America/Winnipeg"), Inclusivity(false, true))

julia> r[1:2]
Error showing value of type StepRangeLen{AnchoredInterval{-1 hour,ZonedDateTime},AnchoredInterval{-1 hour,ZonedDateTime},Hour}:
ERROR: MethodError: Cannot `convert` an object of type Hour to an object of type ZonedDateTime
Closest candidates are:
  convert(::Type{T}, ::Interval{T}) where T at /Users/ericdavies/.julia/packages/Intervals/52k6l/src/interval.jl:124
  convert(::Type{T}, ::AnchoredInterval{P,T}) where {P, T} at /Users/ericdavies/.julia/packages/Intervals/52k6l/src/anchoredinterval.jl:174
  convert(::Type{T}, ::T) where T at essentials.jl:167
  ...
Stacktrace:
 [1] AnchoredInterval{-1 hour,ZonedDateTime}(::Hour) at /Users/ericdavies/.julia/packages/Intervals/52k6l/src/anchoredinterval.jl:79
 [2] step(::StepRangeLen{AnchoredInterval{-1 hour,ZonedDateTime},AnchoredInterval{-1 hour,ZonedDateTime},Hour}) at ./range.jl:501
 [3] show(::IOContext{REPL.Terminals.TTYTerminal}, ::StepRangeLen{AnchoredInterval{-1 hour,ZonedDateTime},AnchoredInterval{-1 hour,ZonedDateTime},Hour}) at ./range.jl:712
 [4] show(::IOContext{REPL.Terminals.TTYTerminal}, ::MIME{Symbol("text/plain")}, ::StepRangeLen{AnchoredInterval{-1 hour,ZonedDateTime},AnchoredInterval{-1 hour,ZonedDateTime},Hour}) at ./show.jl:7
 [5] display(::REPL.REPLDisplay{REPL.LineEditREPL}, ::MIME{Symbol("text/plain")}, ::StepRangeLen{AnchoredInterval{-1 hour,ZonedDateTime},AnchoredInterval{-1 hour,ZonedDateTime},Hour}) at /Users/ericdavies/.julia/packages/OhMyREPL/GFHgr/src/output_prompt_overwrite.jl:6
 [6] display(::REPL.REPLDisplay, ::Any) at /Users/ericdavies/repos/julia1p2/usr/share/julia/stdlib/v1.2/REPL/src/REPL.jl:136
 [7] display(::Any) at ./multimedia.jl:323

Range indexing appears to be taking a StepRange and making a StepRangeLen, which we don't handle effectively.

I think this might be a bug in step(::StepRangeLen).

@iamed2
Copy link
Member Author

iamed2 commented Nov 18, 2019

I think that bug is there, but we can also avoid it by never making a StepRangeLen. We can prevent that by defining the traits OrderStyle and ArithmeticStyle to be the same as AbstractTime. We should also add RangeStepStyle while we're at it.

@omus
Copy link
Collaborator

omus commented Feb 25, 2020

As of Julia 1.5.0-DEV.207 (7d92a3aaed) this issue appears fixed:

julia> r[1:2]
AnchoredInterval{Hour(-1),ZonedDateTime}(ZonedDateTime(2014, 1, 1, tz"America/Winnipeg"), Inclusivity(false, true)):Hour(1):AnchoredInterval{Hour(-1),ZonedDateTime}(ZonedDateTime(2014, 1, 1, 1, tz"America/Winnipeg"), Inclusivity(false, true))

Do we want to try and fix this on earlier versions of Julia?

@iamed2
Copy link
Member Author

iamed2 commented Feb 25, 2020

We should figure out the correct traits. If that fixes it for older versions of Julia, that would be a bonus.

@omus
Copy link
Collaborator

omus commented Jul 20, 2020

Another example of this issue but on Intervals 1.3 you just see a deprecation warning:

julia> using Intervals

julia> x = AnchoredInterval{-1}(1):1:AnchoredInterval{-1}(5)
AnchoredInterval{-1,Int64,Open,Closed}(1):1:AnchoredInterval{-1,Int64,Open,Closed}(5)

julia> y = x[1:4];

julia> y
┌ Warning: `convert(T, interval::AnchoredInterval{P,T})` is deprecated for intervals which are not closed with coinciding endpoints. Use `anchor(interval)` instead.
│   caller = AnchoredInterval at anchoredinterval.jl:95 [inlined]
└ @ Core ~/.julia/dev/Intervals/src/anchoredinterval.jl:95
┌ Warning: `convert(T, interval::AnchoredInterval{P,T})` is deprecated for intervals which are not closed with coinciding endpoints. Use `anchor(interval)` instead.
│   caller = AnchoredInterval at anchoredinterval.jl:95 [inlined]
└ @ Core ~/.julia/dev/Intervals/src/anchoredinterval.jl:95
AnchoredInterval{-1,Int64,Open,Closed}(1):AnchoredInterval{-1,Int64,Open,Closed}(1):AnchoredInterval{-1,Int64,Open,Closed}(4)

This was referenced Jul 20, 2020
@omus
Copy link
Collaborator

omus commented Jul 20, 2020

I attempted to use traits to fix this problem but that didn't actually seem to be the source of the failure. I instead implemented the fix in #132 and made a new issue for implementing traits.

@omus omus closed this as completed in #132 Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants