-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: promote ranges to the largest of the the start, step, or length (as applicable) #43059
Conversation
75f1cf0
to
ff185b7
Compare
f59082e
to
401fd1e
Compare
From triage: looks pretty reasonable. But, we would really like to have the property that requesting a length of 0 (of any type) gives a fully-functional empty range. That requires us to switch from UnitRange to StepRangeLen as the return type for some ranges. Jameson will try to implement that, and we'll see if it breaks anything. |
401fd1e
to
c55dbc2
Compare
c55dbc2
to
ffda97d
Compare
merge? |
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
@vtjnash CI is green, but looks like there are some potential benchmark regressions? |
looks like just noise to me |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
We cannot safely use reverse, so we do not anymore. That is now causing conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways. Closes JuliaLang#43788
We cannot safely use reverse, so we do not anymore. That is now causing conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways. Closes JuliaLang#43788
Not sure how much this is to worry about and it might be better fixed over there, but this broke DSP.jl because it does convert(StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}}, range(0.0, step=1, length=10)) which now throws. Including the final type argument, i.e. convert(StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int}, range(0.0, step=1, length=10)) still works, but obviously requires Julia 1.7 or later. |
In Julia 1.7, the defintion of `StepRangeLen` acquired another type parameter, making the fields typed with `FloatRange{Float64}` abstractly typed. While that is unfortunate but not critical, JuliaLang/julia#43059 further broke `convert`ing to this abstract type, causing failures within DSP.jl. This change reworks the defintion of `FloatRange` to pick the type of an appropriate `range` call, making it adjust automatically to the changes in Julia. To simplify things, the type parameter is dropped, as it was only used with `Float64` anyway.
In Julia 1.7, the definition of `StepRangeLen` acquired another type parameter, making the fields typed with `FloatRange{Float64}` abstractly typed. While that is unfortunate but not critical, JuliaLang/julia#43059 further broke `convert`ing to this abstract type, causing failures within DSP.jl. This change reworks the definition of `FloatRange` to pick the type of an appropriate `range` call, making it adjust automatically to the changes in Julia. To simplify things, the type parameter is dropped, as it was only used with `Float64` anyway.
In Julia 1.7, the definition of `StepRangeLen` acquired another type parameter, making the fields typed with `FloatRange{Float64}` abstractly typed. While that is unfortunate but not critical, JuliaLang/julia#43059 further broke `convert`ing to this abstract type, causing failures within DSP.jl. This change reworks the definition of `FloatRange` to pick the type of an appropriate `range` call, making it adjust automatically to the changes in Julia. To simplify things, the type parameter is dropped, as it was only used with `Float64` anyway.
That seems like it should rather have been EDIT: I see that DSP.jl went with the |
…pplicable) (JuliaLang#43059) Be careful to use `oneunit` instead of `1`, so that arithmetic on user-given types does not promote first to Int. Fixes JuliaLang#35711 Fixes JuliaLang#10554
We cannot safely use reverse, so we do not anymore. That is now causing conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways. Closes JuliaLang#43788
This feels like a PR where a PkgEval run would have been useful. |
That is a bug, and I thought we already fixed it on master |
Ah, we only fixed the adjacent case for float steps, but missed that |
So is jw3126/RangeHelpers.jl#2 an issue on Julia or expected? |
We should fix it here. @jipolanco would you like to tackle this too, since you fixed the similar case in #44313? |
…pplicable) (JuliaLang#43059) Be careful to use `oneunit` instead of `1`, so that arithmetic on user-given types does not promote first to Int. Fixes JuliaLang#35711 Fixes JuliaLang#10554
We cannot safely use reverse, so we do not anymore. That is now causing conflict between JuliaLang#43059 and JuliaLang#29842. And this is likely clearer anyways. Closes JuliaLang#43788
Right, in #44313 I only took care of float steps. I'll try to push a fix for the general case. |
This is a test to see if we could switch
range
fromoftype
topromote
for computing the size of the range. The main area this disrupted in the tests were caused by the auto-promotion rules for a few smallint types, causinglength(r)
to be a larger type than expected, and necessitating computingstop
directly instead (which is an exact computation for these types, though would not have been valid generally–but other types generally do this promotion inside the definition oflength
).Fixes #35711
Fixes #10554