Skip to content

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Mar 20, 2025

Currently, the implementation of no_offset_view for a Slice doesn't actually ensure that there is no offset. This is because Base.Slice(UnitRange(a)) has UnitRange(a) as its axes, which are usually offset. This PR changes the behavior to return a UnitRange(a) instead.

Fixes #375

The flip side of the current implementation is that the Slice information is lost, so IndexStyle of 2D views might become IndexCartesian once they are routed through no_offset_view.

julia> arr = OffsetArray(reshape(1:15, 3, 5), 2, 3)
3×5 OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8) with eltype Int64 with indices 3:5×4:8:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> V = view(arr, :, :)
3×5 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), :, :) with eltype Int64 with indices 3:5×4:8:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> IndexStyle(V)
IndexLinear()

julia> V_nooff = OffsetArrays.no_offset_view(V)
3×5 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), 3:5, 4:8) with eltype Int64:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> IndexStyle(V_nooff)
IndexCartesian()

An alternate approach might be to shift the parent to one-based indices, and use Base.Slice{Base.OneTo{Int}} axes, which preserve this information. This, however, departs from the current approach of preserving the parentindices of the SubArray.

Edit: the alternate approach is implemented now.

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.69%. Comparing base (497deea) to head (284625d).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   95.53%   95.69%   +0.16%     
==========================================
  Files           6        6              
  Lines         448      465      +17     
==========================================
+ Hits          428      445      +17     
  Misses         20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jishnub
Copy link
Member Author

jishnub commented Mar 20, 2025

In 96d0f15, I have explored the alternate idea, where we shift the indices of the parent as well as the parentindices of the view:

julia> arr = OffsetArray(reshape(1:15, 3, 5), 2, 3)
3×5 OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8) with eltype Int64 with indices 3:5×4:8:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> V = view(arr, :, :)
3×5 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), :, :) with eltype Int64 with indices 3:5×4:8:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> V_nooff = no_offset_view(V) # unwraps the `view` as well as the `OffsetArray`
3×5 reshape(::UnitRange{Int64}, 3, 5) with eltype Int64:
 1  4  7  10  13
 2  5  8  11  14
 3  6  9  12  15

julia> IndexStyle(V_nooff) # retains the `IndexStyle`
IndexLinear()

julia> V = view(arr, :, 4:4)
3×1 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), :, 4:4) with eltype Int64 with indices OffsetArrays.IdOffsetRange(values=3:5, indices=3:5)×Base.OneTo(1):
 1
 2
 3

julia> no_offset_view(V) # changes the `parentindices` as well as the indices of the `parent`, but the axis of the view is still a `Slice`
3×1 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 1:3, 4:8), :, 4:4) with eltype Int64:
 1
 2
 3

julia> V = @view arr[begin:end, 4:4]
3×1 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), 3:5, 4:4) with eltype Int64:
 1
 2
 3

julia> no_offset_view(V) # no change if there isn't a `Slice` as an axis
3×1 view(OffsetArray(reshape(::UnitRange{Int64}, 3, 5), 3:5, 4:8), 3:5, 4:4) with eltype Int64:
 1
 2
 3

@jishnub jishnub merged commit 73f7acd into JuliaArrays:master Apr 2, 2025
17 checks passed
@jishnub jishnub deleted the jishnub/no_offset_view_subarary branch April 2, 2025 07: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.

no_offset_view does not work properly on views with :

1 participant