RFC: promote ranges to the largest of the the start, step, or length (as applicable) #43059
Conversation
vtjnash
force-pushed
the
jn/ranges-last
branch
from
75f1cf0
to
ff185b7
Compare
vtjnash
force-pushed
the
jn/bigstep-len-overflow
branch
from
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. |
vtjnash
force-pushed
the
jn/bigstep-len-overflow
branch
from
401fd1e
to
c55dbc2
Compare
vtjnash
force-pushed
the
jn/bigstep-len-overflow
branch
from
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. |
DilumAluthge
removed
the
merge me
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
rangefromoftypetopromotefor 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 computingstopdirectly 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