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

Fix linear indexing for ReshapedArray if the parent has offset axes #41232

Merged
merged 3 commits into from
Mar 11, 2024

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Jun 15, 2021

Fixes #39953

Currently a ReshapedArray is too permissive, and it acts as an offset-free wrapper if the parent is an offset array.

julia> r = Base.IdentityUnitRange(0:1)
Base.IdentityUnitRange(0:1)

julia> reshape(r, (2,1))
2×1 reshape(::Base.IdentityUnitRange{UnitRange{Int64}}, 2, 1) with eltype Int64:
 0
 1

This causes multiple problems -- firstly linear indexing is broken for such an array (#39953). Secondly, if a 1D offset array is wrapped in a 2D ReshapedArray, calling vec on it fails, as the operation is a reshape on the parent which does not have 1-based indices.

julia> reshape(r, (2,1)) |> vec
ERROR: ArgumentError: offset arrays are not supported but got an array with index other than 1
Stacktrace:
 [1] require_one_based_indexing
   @ ./abstractarray.jl:110 [inlined]
 [2] _reshape
   @ ./reshapedarray.jl:168 [inlined]
 [3] _reshape
   @ ./reshapedarray.jl:186 [inlined]
 [4] reshape
   @ ./reshapedarray.jl:112 [inlined]
 [5] reshape
   @ ./reshapedarray.jl:116 [inlined]
 [6] vec
   @ ./abstractarraymath.jl:41 [inlined]
 [7] |>(x::Base.ReshapedArray{Int64, 2, Base.IdentityUnitRange{UnitRange{Int64}}, Tuple{}}, f::typeof(vec))
   @ Base ./operators.jl:966
 [8] top-level scope
   @ REPL[6]:1

While the linear indexing may be fixed by adding appropriate offsets to the indices, fixing all the problems associated with this might be more difficult.

This PR makes reshaping an offset array illegal. A custom offset array type should define its own reshape method to specify what's to be done with the axes offsets. Often, eg. in OffsetArrays, the reshape may be translated to the parent, and new offsets may be wrapped around the result.

This PR fixes the linear indexing issue. The vec one is not resolved.

@mcabbott
Copy link
Contributor

it acts as an offset-free wrapper

Isn't this what it should do? Once you have offsets around, similar(o, Int, (2,)) is best viewed as shorthand for similar(o, Int, (Base.OneTo(2),)), and likewise reshape(o, 2). That's what happens in these examples, which this PR would (I think) make into errors?

julia> ot = OffsetArray(rand(Int8,3,2),1,10)'
2×3 adjoint(OffsetArray(::Matrix{Int8}, 2:4, 11:12)) with eltype Int8 with indices 11:12×2:4:
 -121   53  -16
   88  118   43

julia> ot[1], ot[5] # linear indexing
(-121, -16)

julia> vec(ot)[1], vec(ot)[5]
(-121, -16)

julia> reshape(ot, 3,2)
3×2 reshape(adjoint(OffsetArray(::Matrix{Int8}, 2:4, 11:12)), 3, 2) with eltype Int8:
 -121  118
   88  -16
   53   43

julia> ans[1]
-121

julia> @which reshape(ot, 3,2)  # does not require OffsetArrays to overload all possible wrappers, just works
reshape(parent::AbstractArray, dims::Int64...) in Base at reshapedarray.jl:116

Likewise for 1D offset arrays:

julia> o = OffsetArray(rand(Int8,4),10)
4-element OffsetArray(::Vector{Int8}, 11:14) with eltype Int8 with indices 11:14:
  -90
  -89
 -103
  127

julia> rm = reshape(o,2,2)  # this is what was requested
2×2 Matrix{Int8}:
 -90  -103
 -89   127

julia> rm[1]
-90

julia> rv = reshape(view(o,:),2,2)
2×2 reshape(view(OffsetArray(::Vector{Int8}, 11:14), :), 2, 2) with eltype Int8:
 -90  -103
 -89   127
 
julia> rv[1]  # that's a bug, surely
0

Can't the linear indexing bug be fixed without breaking these other things?

@jishnub
Copy link
Contributor Author

jishnub commented Jun 15, 2021

Good point, I had overlooked this. The linear indexing bug is easy to fix, and was my original plan. The vec is harder to fix, but that may be addressed separately

@jishnub jishnub changed the title reshape should not accept offset arrays Fix linear indexing for ReshapedArray if the parent has offset axes Jun 19, 2021
@jishnub
Copy link
Contributor Author

jishnub commented Mar 9, 2024

For easier review, this PR currently only fixes

julia> r = reshape(Base.IdentityUnitRange(3:4), 2, 1)
2×1 reshape(::Base.IdentityUnitRange{UnitRange{Int64}}, 2, 1) with eltype Int64:
 3
 4

julia> collect(r) == r
false

julia> collect(r)
2×1 Matrix{Int64}:
 3258125826116431922
 3688512103538242609

After this PR,

julia> collect(r)
2×1 Matrix{Int64}:
 3
 4

@jishnub jishnub requested review from ViralBShah and removed request for ViralBShah March 9, 2024 14:51
@jishnub jishnub merged commit 6e3044d into JuliaLang:master Mar 11, 2024
7 checks passed
@jishnub jishnub deleted the reshapeindexing branch March 11, 2024 06:14
@jishnub jishnub added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 labels Mar 11, 2024
This was referenced Mar 12, 2024
KristofferC pushed a commit that referenced this pull request Mar 16, 2024
…41232)

This PR fixes
```julia
julia> r = reshape(Base.IdentityUnitRange(3:4), 2, 1)
2×1 reshape(::Base.IdentityUnitRange{UnitRange{Int64}}, 2, 1) with eltype Int64:
 3
 4

julia> collect(r) == r
false

julia> collect(r)
2×1 Matrix{Int64}:
 3258125826116431922
 3688512103538242609
```

After this PR,
```julia
julia> collect(r)
2×1 Matrix{Int64}:
 3
 4
```

(cherry picked from commit 6e3044d)
KristofferC added a commit that referenced this pull request Mar 17, 2024
Backported PRs:
- [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type -->
- [x] #51802 <!-- Allow AnnotatedStrings in log messages -->
- [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays -->
- [x] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [x] #53482 <!-- add IR encoding for EnterNode -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation
-->
- [x] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [x] #53523 <!-- add back an alias for `check_top_bit` -->
- [x] #53377 <!-- add _readdirx for returning more object info gathered
during dir scan -->
- [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure
-->
- [x] #53540 <!-- use more efficient `_readdirx` for tab completion -->
- [x] #53545 <!-- use `_readdirx` for `walkdir` -->
- [x] #53551 <!-- revert "Add @create_log_macro for making custom styled
logging macros (#52196)" -->
- [x] #53554 <!-- Always return a value in 1-d circshift! of
abstractarray.jl -->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53571 <!-- Update Documenter to v1.3 for inventory writing -->
- [x] #53403 <!-- Move parallel precompilation to Base -->
- [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53606 <!-- fix error path in `precompilepkgs` -->
- [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base -->
- [x] #53629 <!-- typo fix in scoped values docs -->
- [x] #53630 <!-- sroa: Fix incorrect scope counting -->
- [x] #53598 <!-- Use Base parallel precompilation to build stdlibs -->
- [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are
in fact only weak -->
- [x] #53671 <!-- Fix bootstrap Base precompile in cross compile
configuration -->
- [x] #52125 <!-- Load Pkg if not already to reinstate missing package
add prompt -->
- [x] #53602 <!-- Handle zero on arrays of unions of number types and
missings -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53679 <!-- move precompile workload back from Base -->
- [x] #53663 <!-- add isassigned methods for reinterpretarray -->
- [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions
accepted -->
- [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec -->
- [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in
cconvert for Array -->
- [x] #53631 <!-- LAPACK: validate input parameters to throw informative
errors -->
- [x] #53628 <!-- Make some improvements to the Scoped Values
documentation. -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to
`filesystem.jl` -->
- [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent
has offset axes -->
- [x] #53527 <!-- Enable analyzegc checks for try catch and fix found
issues -->
- [x] #52092 
- [x] #53682 <!-- Increase build precompilation -->
- [x] #53720 
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
innervars. -->

Contains multiple commits, manual intervention needed:
- [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex
indices -->

Non-merged PRs with backport label:
- [ ] #53736 <!-- fix literal-pow to return the right type when the base
is -1 -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [ ] #53660 <!-- put Logging back in default sysimage -->
- [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51928 <!-- Styled markdown, with a few tweaks -->
- [ ] #51816 <!-- User-themable stacktraces -->
- [ ] #51811 <!-- Make banner size depend on terminal size -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 18, 2024
KristofferC pushed a commit that referenced this pull request Mar 27, 2024
…41232)

This PR fixes
```julia
julia> r = reshape(Base.IdentityUnitRange(3:4), 2, 1)
2×1 reshape(::Base.IdentityUnitRange{UnitRange{Int64}}, 2, 1) with eltype Int64:
 3
 4

julia> collect(r) == r
false

julia> collect(r)
2×1 Matrix{Int64}:
 3258125826116431922
 3688512103538242609
```

After this PR,
```julia
julia> collect(r)
2×1 Matrix{Int64}:
 3
 4
```

(cherry picked from commit 6e3044d)
@KristofferC KristofferC mentioned this pull request May 8, 2024
23 tasks
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label May 8, 2024
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
…uliaLang#41232)

This PR fixes
```julia
julia> r = reshape(Base.IdentityUnitRange(3:4), 2, 1)
2×1 reshape(::Base.IdentityUnitRange{UnitRange{Int64}}, 2, 1) with eltype Int64:
 3
 4

julia> collect(r) == r
false

julia> collect(r)
2×1 Matrix{Int64}:
 3258125826116431922
 3688512103538242609
```

After this PR,
```julia
julia> collect(r)
2×1 Matrix{Int64}:
 3
 4
```

(cherry picked from commit 6e3044d)
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.

Linear indexing of a ReshapedArray is broken for non 1-based parent arrays
3 participants