Skip to content

Fix #39798 by using one(start) rather than 1 for _linrange#40467

Merged
oscardssmith merged 4 commits intoJuliaLang:masterfrom
mkitti:fix_39798_one
May 27, 2021
Merged

Fix #39798 by using one(start) rather than 1 for _linrange#40467
oscardssmith merged 4 commits intoJuliaLang:masterfrom
mkitti:fix_39798_one

Conversation

@mkitti
Copy link
Copy Markdown
Contributor

@mkitti mkitti commented Apr 13, 2021

This is a variation on #39808 using one(start) rather than oneunit(start) and fixes the bug identified in #39798:

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}, ::Base.TwicePrecision) where T<:Number at twiceprecision.jl:250
  ...
Stacktrace:
 [1] Base.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
...

In the hypothetical scenario proposed by @mbauman, start is a unitful Integer. In that case, one(start) is better for den than oneunit(start) where start <: Integer because one(start) is unitless and would not strip the units of the ratio. Thus, start and one(start) have distinct types since the former is unitful and the latter is unitless. Thus the method TwicePrecision{T}(nd::Tuple{I,I}, nb::Integer) where {T,I} would not apply.

In this version, we thus also implement TwicePrecision{T}(nd::Tuple{Integer,Integer}, nb::Integer) where T.

@mkitti
Copy link
Copy Markdown
Contributor Author

mkitti commented Apr 25, 2021

@mbauman After thinking about it for some time, I think we should merge this one to fix #39798. I'm quite confident that the one vs oneunit matter makes little difference and is otherwise quite hard to discern without a concrete scenario. one seems like the minimal fix for the issue.

The additional TwicePrecision method introduced in this PR also independently resolves the issue as demonstrated below.

julia> function Base.TwicePrecision{T}(nd::Tuple{Integer,Integer}, nb::Integer) where T
           Base.twiceprecision(Base.TwicePrecision{T}(nd), nb)
       end

julia> range(Int32(1), Int32(1); length = 10)
1.0:0.0:1.0

julia> typeof(range(Int32(1), Int32(1); length = 10))
StepRangeLen{Float64, Base.TwicePrecision{Float64}, Base.TwicePrecision{Float64}}

@oscardssmith oscardssmith merged commit d2b2b8c into JuliaLang:master May 27, 2021
shirodkara pushed a commit to shirodkara/julia that referenced this pull request Jun 9, 2021
…uliaLang#40467)

* Fix JuliaLang#39798 by using one(start) rather than 1 for _linrange
* Expand tests for JuliaLang#39798
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
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 this pull request may close these issues.

2 participants