-
Notifications
You must be signed in to change notification settings - Fork 34
Non-contiguous block slicing operations #462
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
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #462 +/- ##
==========================================
+ Coverage 93.64% 93.74% +0.10%
==========================================
Files 19 19
Lines 1667 1678 +11
==========================================
+ Hits 1561 1573 +12
+ Misses 106 105 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances support for non-contiguous block slicing by introducing NoncontiguousBlockSlice
, generalizing BlockIndex
/BlockIndexRange
to accept vector indices, and updating views logic and tests accordingly.
- Adds
NoncontiguousBlockSlice
type and integrates it into slicing routines - Broadens
BlockIndex
/BlockIndexRange
constructors andgetindex
to handle arbitrary vectors - Expands test coverage for non-contiguous and nested block slices
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
test/test_blockviews.jl | Fixes test name typo and adds tests for NoncontiguousBlockSlice |
test/test_blockindices.jl | Imports new slice type, extends BlockIndex tests, adds vector-index tests |
test/test_blockarrays.jl | Adds tests for non-contiguous and nested block indexing |
src/views.jl | Updates _blockslice , constants, and reindex to use new type |
src/blockindices.jl | Broadens type parameters, extends constructors, defines new slice type and methods |
Comments suppressed due to low confidence (2)
src/blockindices.jl:296
- [nitpick] The docstring could be clarified by fixing indentation and explicitly documenting the roles of the
block
vsblocks
parameters, and adding examples of valid index types.
Represents an AbstractVector of indices attached to a (potentially non-contiguous) subblock,
test/test_blockviews.jl:334
- The test set name was corrected from "blockrange-of-blockreange" to "blockrange-of-blockrange"; please ensure other references are updated if any.
@testset "blockrange-of-blockrange" begin
@@ -198,20 +198,20 @@ represents a cartesian range inside a block. | |||
""" | |||
BlockIndexRange | |||
|
|||
BlockIndexRange(block::Block{N}, inds::Vararg{AbstractUnitRange{<:Integer},N}) where {N} = | |||
BlockIndexRange(block::Block{N}, inds::Vararg{AbstractVector,N}) where {N} = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generalization enables constructing non-contiguous subblock slices such as Block(2)[[1, 3]]
. My biggest concern about this change is that BlockIndexRange
is kind of a strange name for that object since it isn't a range. I can't really think of a better name though. One thing to consider could be renaming this struct to something more general like BlockIndexVector
(or maybe BlockIndices
, though that clashes with an alternative definition of BlockIndices
proposed in #356) and then define BlockIndexRange
as a type alias where the indices are constrained to AbstractUnitRange{<:Integer}
.
@@ -140,23 +140,23 @@ julia> a[BlockIndex((2,2), (2,3))] | |||
20 | |||
``` | |||
""" | |||
struct BlockIndex{N,TI<:Tuple{Vararg{Integer,N}},Tα<:Tuple{Vararg{Integer,N}}} | |||
struct BlockIndex{N,TI<:Tuple{Vararg{Any,N}},Tα<:Tuple{Vararg{Any,N}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generalization enables constructing indexing objects such as Block(1)[Block(2)]
or Block(1)[Block(2)[3]]
, for example in the case when a block array has blocks that are themselves blocked. I have use cases for that where I want to construct a block array where the blocks have extra structure and custom non-integer indexing (in particular, the blocks have a Kronecker product structure, so I want to be able to slice the blocks while preserving that structure).
|
||
This mimics the relationship between `Colon` and `Base.Slice`, `Block` and `BlockSlice`, etc. | ||
""" | ||
struct NoncontiguousBlockSlice{BB,T,INDS<:AbstractVector{T}} <: AbstractVector{T} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a relaunch of #459, hopefully with a better name, more context, and a better docstring. The idea is that BlockSlice
is analogous to Slice
for contiguous blockwise slices, for example when you slice with Block(2)
, Block(2)[1:2]
, and Block.(1:2)
those get converted to BlockSlice
by to_indices
in order to store both the contiguous absolute range but also preserve the original blockwise information. Here, a NoncontiguousBlockSlice
gets constructed by to_indices
when you slice with non-contiguous blockwise slices such as [Block(1), Block(3)]
, [Block(1)[1:2], Block(3)[2:3]]
, and Block(2)[[2, 4]]
.
The reason for having two types is basically for the purpose of subyping: BlockSlice
is an AbstractUnitRange
and that information is used in a various parts of code, while this covers more general cases where the slices are not equivalent to unit ranges.
This adds more support for non-contiguous block slicing operations. In particular, it generalizes
BlockIndex
andBlockIndexRange
so that slicing likeA[Block(1)[[1, 3]]]
works, and additionally in the case where blocks themselves are blocked, it enables slicing likeA[Block(1)[Block(2)]]
.This supersedes #459 and closes #355. Also closes #419.