Fix #39798 by using oneunit(start) rather than 1 for _linrange#39808
Fix #39798 by using oneunit(start) rather than 1 for _linrange#39808mkitti wants to merge 4 commits intoJuliaLang:masterfrom
1 for _linrange#39808Conversation
|
Should this just be |
|
|
|
I added the following test set: @testset "Non-Int64 endpoints that are identical (#39798)" begin
for T in DataType[Float64,Int8,Int16,Int32,Int64,Int128,UInt8,UInt16,UInt32,UInt64],
r in [ LinRange(1, 1, 10), StepRangeLen(7, 0 , 5) ]
let start=T(first(r)), stop=T(last(r)), step=T(step(r)), length=length(r)
@test range( start, stop, length) == r
@test range( start, stop; length) == r
@test range( start; stop, length) == r
@test range(; start, stop, length) == r
@test range( start; step, length) == r
@test range(; start, step, length) == r
@test range(; stop, step, length) == r
end
end
endOn 1.5.3, it fails: On this branch, it succeeds: |
|
I think we want |
3cd950d to
1cb651d
Compare
one vs oneunitRegarding Thus, the choice has to be informed by method dispatch. Let's examine the stack trace and see where this fails. Stack Trace [1] Base.TwicePrecision{Float64}(::Tuple{Int32,Int64}, ::Int64) at ./twiceprecision.jl:185
[2] steprangelen_hp(::Type{Float64}, ::Tuple{Int32,Int64}, ::Tuple{Int32,Int64}, ::Int64, ::Int64, ::Int64) at ./twiceprecision.jl:323
[3] _linspace(::Type{Float64}, ::Int32, ::Int32, ::Int64, ::Int64) at ./twiceprecision.jl:666
[4] _linspace at ./twiceprecision.jl:663 [inlined]
The intention is for AnalysisI would think that julia> zero(Dates.Day(1))
0 days
julia> one(Dates.Day(1))
1
julia> oneunit(Dates.Day(1))
1 day
julia> using Unitful
julia> zero(1u"g")
0 g
julia> one(1u"g")
1
julia> oneunit(1u"g")
1 gEmpirically, Regarding the matter of stripping units as brought up by @mbauman , this code path seems to clearly intended for unitless ranges of Unitful.jl creates unit based ranges by stripping the units, creating a range, and then adding them back in. Alternative ApproachAt the end of the day, this is all trying to return This alternative approach could be pursued in a separate pull request in addition to the current fix which is still applicable even if a ConclusionBased on the current flow of the code in terms of the dispatched method, Thus, we should proceed with merging the current approach. |
|
Can we add a bugfix label here? |
|
@mbauman Any further thoughts on this? Do you still still think |
|
It's not often that I disagree with Tim, but yes, I still think that The question isn't so much if I still think so, but rather if I managed to convince @timholy. :) |
|
If not, what you have is just fine; this really is a distinction without a (currently-observable) difference. |
I think we may have made this too complicated. Ultimately, we are talking about |
|
The dispatch analysis I did above suggests that the intended method to invoke is Thus In this purely hypothetical scenario, if we used The problem with |
|
An alternative fix (or in combination with julia> range(Int32(1), Int32(1), length = 10)
ERROR: MethodError: Cannot `convert` an object of type Tuple{Int32, Int64} to an object of type Float64
Closest candidates are:
convert(::Type{T}, ::T) where T<:Number at number.jl:6
convert(::Type{T}, ::Number) where T<:Number at number.jl:7
convert(::Type{T}, ::TwicePrecision) where T<:Number at twiceprecision.jl:250
...
Stacktrace:
[1] TwicePrecision{Float64}(hi::Tuple{Int32, Int64}, lo::Int64)
@ Base .\twiceprecision.jl:185
[2] steprangelen_hp(#unused#::Type{Float64}, ref::Tuple{Int32, Int64}, step::Tuple{Int32, Int64}, nb::Int64, len::Int64, offset::Int64)
@ Base .\twiceprecision.jl:323
[3] _linspace(#unused#::Type{Float64}, start_n::Int32, stop_n::Int32, len::Int64, den::Int64)
@ Base .\twiceprecision.jl:663
[4] _linspace
@ .\twiceprecision.jl:660 [inlined]
[5] _range
@ .\range.jl:441 [inlined]
[6] _range2
@ .\range.jl:100 [inlined]
[7] #range#58
@ .\range.jl:94 [inlined]
[8] top-level scope
@ REPL[2]:1
julia> import Base: TwicePrecision, twiceprecision
julia> function TwicePrecision{T}(nd::Tuple{Integer,Integer}, nb::Integer) where {T}
twiceprecision(TwicePrecision{T}(nd), nb)
end
julia> range(Int32(1), Int32(1), length = 10)
1.0:0.0:1.0Anyways, I think it would be great for |
|
I implemented the |
Fixes #39798 by using
oneunit.