Skip to content

Conversation

@jishnub
Copy link
Member

@jishnub jishnub commented Jan 14, 2021

This PR adds an extra function offset_view and macro @offset_view that mirror Base.view, except they preserve the indices used for the original indexing. This is achieved by wrapping the view in an OffsetArray.
These functions are complementary no_offset_view

julia> OffsetArrays.@offset_view (1:10)[3:4]
3:4 with indices 3:4

julia> a = reshape(1:12, 3, 4)
3×4 reshape(::UnitRange{Int64}, 3, 4) with eltype Int64:
 1  4  7  10
 2  5  8  11
 3  6  9  12

julia> OffsetArrays.@offset_view a[:, 3]
3-element OffsetArray(view(reshape(::UnitRange{Int64}, 3, 4), :, 3), 1:3) with eltype Int64 with indices 1:3:
 7
 8
 9

julia> OffsetArrays.@offset_view a[2:3, 3:4]
2×2 OffsetArray(view(reshape(::UnitRange{Int64}, 3, 4), 2:3, 3:4), 2:3, 3:4) with eltype Int64 with indices 2:3×3:4:
 8  11
 9  12

Unfortunately this only works with index types that are compatible with the OffsetArray constructor, such as AbstractUnitRanges, CartesianIndices and such, and doesn't work for Vectors and other dense index ranges. For example:

julia> OffsetArrays.@offset_view a[2:3, [3,4]]
ERROR: MethodError: Cannot `convert` an object of type Array{Int64,1} to an object of type AbstractUnitRange{Int64}

(The error message may be improved).

There is another way to construct such an index-preserving view: by using IdOffsetRange axes in view(A, indices...). Personally I would prefer this, as it returns a SubArray with axes that are offset, but as #186 show this has approach is still buggy.

@codecov
Copy link

codecov bot commented Jan 14, 2021

Codecov Report

Merging #187 (3b3f95e) into master (94a4172) will decrease coverage by 0.62%.
The diff coverage is 86.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #187      +/-   ##
==========================================
- Coverage   98.91%   98.29%   -0.63%     
==========================================
  Files           5        5              
  Lines         277      293      +16     
==========================================
+ Hits          274      288      +14     
- Misses          3        5       +2     
Impacted Files Coverage Δ
src/utils.jl 100.00% <ø> (ø)
src/OffsetArrays.jl 97.96% <83.33%> (-0.95%) ⬇️
src/axes.jl 97.95% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 94a4172...3b3f95e. Read the comment docs.

@jishnub
Copy link
Member Author

jishnub commented Jan 14, 2021

This PR is branched off from #185 as that bugfix was necessary for this, so perhaps I can rebase this against master after that is merged. This might make it easier to review.

@timholy
Copy link
Member

timholy commented Jan 14, 2021

I'm a little skeptical about this. Let's just fix the bug in Base. (I'm busy with other things right now, sadly, so I haven't even looked.)

EDIT: with #185 fixed correctly perhaps it would be worth pushing a "refreshed" version and then I can review.

@jishnub
Copy link
Member Author

jishnub commented Jan 18, 2021

This is perhaps not necessary, as an index-preserving subarray may be achieved using a Base.IdentityUnitRange.

julia> a = reshape(1:12, 3, 4)
3×4 reshape(::UnitRange{Int64}, 3, 4) with eltype Int64:
 1  4  7  10
 2  5  8  11
 3  6  9  12

julia> @view a[Base.IdentityUnitRange(2:3), 2]
2-element view(reshape(::UnitRange{Int64}, 3, 4), :, 2) with eltype Int64 with indices 2:3:
 5
 6

Given that the wrapper only applies to AbstractUnitRanges and not to a generic indexing operation, perhaps we do not need to include it here.

@timholy
Copy link
Member

timholy commented Jan 18, 2021

That's how I've always done it, and as you point out it's much more flexible because you can apply it to a subset of axes.

I'm happy having this closed.

@jishnub jishnub closed this Jan 18, 2021
@jishnub jishnub deleted the oview branch August 18, 2021 08:10
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