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

LinRange{Int}(...) allows creating a LinRange that doesn't really work #35219

Closed
simeonschaub opened this issue Mar 22, 2020 · 7 comments
Closed
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior help wanted Indicates that a maintainer wants help on an issue or pull request

Comments

@simeonschaub
Copy link
Member

eltype(LinRange{Int}(1, 4, 5)) is Int, whereas the elements are actually of type Float64, since LinRange uses / for division. Furthermore, LinRange{Int}(1, 4, 5) isa AbstractRange{Int} is also technically wrong, since the docstring of AbstractRange clearly states:

  AbstractRange{T}

  Supertype for ranges with elements of type T. UnitRange and other types are subtypes of this.

Thanks to @asinghvi17 for bringing this up on Slack!

@StefanKarpinski StefanKarpinski added arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior help wanted Indicates that a maintainer wants help on an issue or pull request labels Mar 22, 2020
@timholy timholy added the types and dispatch Types, subtyping and method dispatch label Mar 23, 2020
@timholy
Copy link
Member

timholy commented Mar 23, 2020

This is probably not fixable with the current type system. The issue is you need

julia> struct MyLinRange{T} <: AbstractRange{typeof(zero(T)/1)}
           start::T
           stop::T
           len::Int
           lendiv::Int
       end
ERROR: MethodError: no method matching zero(::TypeVar)
Closest candidates are:
  zero(::Type{Missing}) at missing.jl:103
  zero(::Type{LibGit2.GitHash}) at /home/tim/src/julia-1/usr/share/julia/stdlib/v1.4/LibGit2/src/oid.jl:220
  zero(::Type{Pkg.Resolve.VersionWeight}) at /home/tim/src/julia-1/usr/share/julia/stdlib/v1.4/Pkg/src/Resolve/versionweights.jl:15
  ...
Stacktrace:
 [1] top-level scope at REPL[1]:1

Alternatives include adding a second type parameter,

julia> struct MyLinRange{Item,T} <: AbstractRange{T}
           start::Item
           stop::Item
           len::Int
           lendiv::Int

           function MyLinRange{Item,T}(start,stop,len) where {Item,T}
               len >= 0 || throw(ArgumentError("range($start, stop=$stop, length=$len): negative length"))
               if len == 1
                   start == stop || throw(ArgumentError("range($start, stop=$stop, length=$len): endpoints differ"))
                   return new(start, stop, 1, 1)
               end
               new(start,stop,len,max(len-1,1))
           end
       end

julia> MyLinRange{Item}(start, stop, len) where Item = MyLinRange{Item,typeof(start/1)}(start, stop, len)

but I am not sure whether this would be regarded as too breaking.

Note that this issue doesn't come up with LinRange{1, 3, 3) (i.e., if you don't specify the type parameter).

@timholy timholy added this to the 2.0 milestone Mar 23, 2020
@timholy timholy added the triage This should be discussed on a triage call label Mar 23, 2020
@timholy timholy removed this from the 2.0 milestone Mar 23, 2020
@iamed2
Copy link
Contributor

iamed2 commented Mar 23, 2020

The elements of LinRange{Int} are actually Int. The printing is misleading.

julia> l = LinRange{Int}(1,5,4)
4-element LinRange{Int64}:
 1.0,2.33333,3.66667,5.0

julia> l[1]
1

julia> l[end]
5

julia> l[2]
ERROR: InexactError: Int64(2.333333333333333)
Stacktrace:
 [1] Int64 at ./float.jl:710 [inlined]
 [2] lerpi at ./range.jl:687 [inlined]
 [3] unsafe_getindex at ./range.jl:681 [inlined]
 [4] getindex(::LinRange{Int64}, ::Int64) at ./range.jl:666
 [5] top-level scope at REPL[12]:1

@timholy
Copy link
Member

timholy commented Mar 23, 2020

Oh, right, I had forgotten about that. That's a relief. So we should just fix the printing.

@sostock
Copy link
Contributor

sostock commented Mar 23, 2020

In addition to improving the printing, one could also throw an error when creating a LinRange{<:Integer} that would contain non-integer elements. I have a PR (#32439) that does this for StepRange{<:Integer} and StepRangeLen{<:Integer} and will update it to include LinRange.

@JeffBezanson
Copy link
Member

Alternatives include adding a second type parameter,

Yes, I think that would be the right way to do it. (The type parameter can be set by a constructor, and invalid constructor calls can be errors.)

@JeffBezanson JeffBezanson removed the types and dispatch Types, subtyping and method dispatch label Mar 23, 2020
@sostock
Copy link
Contributor

sostock commented Mar 26, 2020

The misleading printing is fixed by #35267.

@JeffBezanson JeffBezanson changed the title eltype(::LinRange{Int}) is wrong LinRange{Int}(...) allows creating a LinRange that doesn't really work Apr 9, 2020
@JeffBezanson
Copy link
Member

Triage thinks this is fixed now. If you explicitly ask for Int elements, you should indeed get an InexactError if the element values can't be converted to that.

@JeffBezanson JeffBezanson removed the triage This should be discussed on a triage call label Apr 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] bug Indicates an unexpected problem or unintended behavior help wanted Indicates that a maintainer wants help on an issue or pull request
Projects
None yet
Development

No branches or pull requests

6 participants