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

Possible breakage in 1.6.0-rc2 and nightly in evaluating CartesianIndices for OffsetArrays with BigInt parent ranges #40035

Closed
jishnub opened this issue Mar 15, 2021 · 6 comments · Fixed by #40038

Comments

@jishnub
Copy link
Contributor

jishnub commented Mar 15, 2021

Unfortunately there's a bug on master in OffsetArrays currently, so this PR is necessary to see the issue:

on 1.5.4:

julia> using OffsetArrays

julia> a = OffsetArray(big(1):big(2), 0);

julia> CartesianIndices(a)
2-element CartesianIndices{1,Tuple{OffsetArrays.IdOffsetRange{Int64,Base.OneTo{Int64}}}} with indices 1:2:
 CartesianIndex(1,)
 CartesianIndex(2,)

on 1.6.0-rc2 and nightly:

julia> CartesianIndices(a)
ERROR: MethodError: no method matching OrdinalRange{Int64, Int64}(::OffsetArrays.IdOffsetRange{BigInt, Base.OneTo{BigInt}})
Closest candidates are:
  OrdinalRange{T1, T2}(::AbstractUnitRange{T1}) where {T1, T2<:Integer} at range.jl:1133
  OrdinalRange{T1, T2}(::StepRange) where {T1, T2<:Integer} at range.jl:1132
  OrdinalRange{T1, T2}(::UnitRange) where {T1, T2<:Integer} at range.jl:1134
  ...
Stacktrace:
 [1] convert(#unused#::Type{OrdinalRange{Int64, Int64}}, r::OffsetArrays.IdOffsetRange{BigInt, Base.OneTo{BigInt}})
   @ Base ./range.jl:215
 [2] (::Base.IteratorsMD.var"#7#8")(r::OffsetArrays.IdOffsetRange{BigInt, Base.OneTo{BigInt}})
   @ Base.IteratorsMD ./multidimensional.jl:271
 [3] map
   @ ./tuple.jl:213 [inlined]
 [4] CartesianIndices(inds::Tuple{OffsetArrays.IdOffsetRange{BigInt, Base.OneTo{BigInt}}})
   @ Base.IteratorsMD ./multidimensional.jl:271
 [5] CartesianIndices(A::OffsetVector{BigInt, UnitRange{BigInt}})
   @ Base.IteratorsMD ./multidimensional.jl:279
 [6] top-level scope
   @ REPL[4]:1

julia> VERSION
v"1.7.0-DEV.707"

This is due to a missing method for the IdOffsetRange axis type defined in OffsetArrays, but this conversion is not used in 1.5.4. The conversion carried out by CartesianIndices appears to have changed from an AbstractUnitRange{Int} in 1.5.4 to an OrdinalRange{Int,Int} in 1.6.

@jishnub jishnub changed the title Possible breakage on nightly in evaluating CartesianIndices for OffsetArrays with BigInt parent ranges Possible breakage in 1.6.0-rc2 and nightly in evaluating CartesianIndices for OffsetArrays with BigInt parent ranges Mar 15, 2021
@jishnub
Copy link
Contributor Author

jishnub commented Mar 15, 2021

Looks like this was introduced by #37829

@KristofferC
Copy link
Sponsor Member

Related to #39589?

@jishnub
Copy link
Contributor Author

jishnub commented Mar 15, 2021

It's not unrelated as the same constructor is called eventually, but this seems to be a different issue (the other one is primarily due to how the integer is converted to a range). This issue can be fixed by adding a specialized CartesianIndices constructor for NTuple{N,AbstractUnitRange{<:Integer}}, so perhaps I will make a PR.

@laborg
Copy link
Contributor

laborg commented Feb 14, 2022

This works on 1.6.5, on 1.7.2 and on master - Is there any reason not to close this (and #40038)?

@KristofferC
Copy link
Sponsor Member

If it works on those releases, it seems indeed a good idea to close.

@vtjnash vtjnash closed this as completed Feb 14, 2022
@vtjnash vtjnash reopened this Feb 14, 2022
@vtjnash
Copy link
Sponsor Member

vtjnash commented Feb 14, 2022

Whoops, I tested that PR and it is still needed. Left a comment there about how we could finish that.

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 a pull request may close this issue.

4 participants