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

Propagate indices in vector indexing of ranges with ranges #41284

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jun 19, 2021

Julia's vector indexing (with OffsetArrays in the mix) follows the rule that axes(r[s]) == axes(s). However, the various getindex methods defined for ranges do not follow this rule, as the result may not be expressed as a range in general if the indices are preserved. OffsetArrays currently carries out extensive type-piracy to "fix" this.

Julia Base:

julia> (1:3)[Base.IdentityUnitRange(2:2)]
2:2

After loading OffsetArrays

julia> (1:3)[Base.IdentityUnitRange(2:2)]
OffsetArrays.IdOffsetRange(values=2:2, indices=2:2)

However this type-piracy of getindex is fragile, since Base (and several packages) dispatch on the first argument of getindex (ie, they define getindex(r::AbstractRange, s)), while OffsetArrays dispatches on the second argument (ie. it defines getindex(r, s::AbstractUnitRange{<:Integer}). This is a minefield of ambiguities, as witnessed recently in JuliaArrays/ArrayInterface.jl#170 (comment).

This PR tries to get around this whole issue by changing how the indices of the indices are propagated. It introduces a function withindices that wraps the result of an indexing operation with the correct axes: withindices(r, axes(s)) has the values of r but the axes of s. Now OffsetArrays only needs to pirate withindices, since the package does not need to compute the values of the result of an indexing operation, only its indices. This implies firstly that fixes to getindex in Base get propagated immediately to OffsetArrays, and secondly OffsetArrays does not introduce ambiguities by dispatching on the second argument.

Currently withindices in Base is a no-op, so it does not change any result and is not a breaking change. The definition of the function really only makes sense if OffsetArrays is loaded, in which case one obtains the correct indices.

Some of the changes in this PR are copied from #41213 as it introduced some fixes for offset ranges. If this PR is merged then the other one might not be necessary. If the other one is merged then this can be rebased on that.

Aside from this there is also a bugfix:

on master:

julia> r = StepRangeLen(Float64(1), Float64(1000), 1000)
1.0:1000.0:999001.0

julia> s = Base.IdentityUnitRange(5:8)
Base.IdentityUnitRange(5:8)

julia> r[s]
ERROR: ArgumentError: StepRangeLen: offset must be in [1,4], got 5

After this PR:

julia> r = StepRangeLen(Float64(1), Float64(1000), 1000)
1.0:1000.0:999001.0

julia> s = Base.IdentityUnitRange(5:8)
Base.IdentityUnitRange(5:8)

julia> r[s]
4001.0:1000.0:7001.0

@jishnub
Copy link
Contributor Author

jishnub commented Jul 1, 2021

Test failure in mpfr seems unrelated. Could someone please review this?

@jishnub jishnub force-pushed the getindexrange branch 2 times, most recently from ff9e3a5 to fff87b1 Compare July 25, 2021 07:51
@jishnub
Copy link
Contributor Author

jishnub commented Jul 27, 2021

Test failure in Sockets in tester_linuxaarch64 seems unrelated

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.

1 participant