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(start::Int; step, length) no longer uses TwicePrecision #44292

Closed
jipolanco opened this issue Feb 21, 2022 · 2 comments · Fixed by #44313
Closed

range(start::Int; step, length) no longer uses TwicePrecision #44292

jipolanco opened this issue Feb 21, 2022 · 2 comments · Fixed by #44313
Labels
ranges Everything AbstractRange regression Regression in behavior compared to a previous version
Milestone

Comments

@jipolanco
Copy link
Contributor

jipolanco commented Feb 21, 2022

Not sure if this is considered a regression, but the following:

r = range(0; step = 0.2, length = 5)
collect(r)  # [0.0, 0.2, 0.4, 0.6, 0.8]                on Julia 1.7
collect(r)  # [0.0, 0.2, 0.4, 0.6000000000000001, 0.8] on Julia 1.8 / master

returns a StepRangeLen{Float64, Int64, Float64, Int64} on Julia master / 1.8, while it used to return a StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}, Int64} on Julia 1.7.

I guess this change of behaviour is in conflict with the statement in the range docs:

Special care is taken to ensure intermediate values are computed rationally. To avoid this induced overhead, see the LinRange constructor.

Note that this doesn't happen when start is a float (more specifically, it must be a Union{Float16,Float32,Float64} and have the same type as step), and therefore changing 0 by 0.0 in the above example works around the issue (so that the range_start_step_length function defined in twiceprecision.jl is called).


This change of behaviour seems to be introduced by #43059, and more precisely in the new definition of the range_start_step_length function copied below. From what I understand, that PR aimed at fixing issues dealing with integer ranges (but I might be wrong about this...). In any case, the branch if stop isa Signed is always skipped if step is a float, in which case stop is also a float.

function range_start_step_length(a, step, len::Integer)
    stop = a + step * (len - oneunit(len))
    if stop isa Signed
        # overflow in recomputing length from stop is okay
        return StepRange{typeof(stop),typeof(step)}(convert(typeof(stop), a), step, stop)
    end
    return StepRangeLen{typeof(stop),typeof(a),typeof(step)}(a, step, len)
end

So, I guess an easy fix would be to add a specialisation such as:

function range_start_step_length(a, step::Union{Float16,Float32,Float64}, len::Integer)
    return range_start_stop_length(oftype(step, a), step, len)
end

which will end up calling the appropriate function from twiceprecision.jl. Is there any reason why this wouldn't be a good idea?

jipolanco added a commit to JuliaVTK/WriteVTK.jl that referenced this issue Feb 21, 2022
@KristofferC KristofferC added this to the 1.8 milestone Feb 21, 2022
@dkarrasch dkarrasch added ranges Everything AbstractRange regression Regression in behavior compared to a previous version labels Feb 21, 2022
@vtjnash
Copy link
Member

vtjnash commented Feb 22, 2022

That sounds right. You are right that I neglected to properly update the range_start_step_length constructor in #43059 to reflect the other changes there.

@jipolanco
Copy link
Contributor Author

Great thanks, I'll make a PR then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ranges Everything AbstractRange regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants