Construct new BlockSkylineMatrix from BlockSkylineMatrix and UnitRanges #229
Conversation
Take an existing BlockSkylineMatrix (or BlockBandedMatrix), and select a sub-matrix by passing UnitRanges for the rows and columns to select, returning a BlockSkylineMatrix (or BlockBandedMatrix).
|
I think I'm going to want a feature like this (actually possibly, eventually, even a more complicated version where I'd allow Needs tests - I can probably work on some if there's interest in merging this PR. Question: I've attempted to make the data copying efficient, but it's pretty ugly, so possibly fragile. Maybe an experienced BlockBandedMatrices.jl developer can see a better way? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #229 +/- ##
==========================================
- Coverage 88.31% 85.23% -3.09%
==========================================
Files 11 11
Lines 1113 1172 +59
==========================================
+ Hits 983 999 +16
- Misses 130 173 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| bi_start = findblockindex.(axes(A), (rows.start, cols.start)) | ||
| bi_stop = findblockindex.(axes(A), (rows.stop, cols.stop)) | ||
|
|
||
| first_block = Int64.(BlockArrays.block.(bi_start)) |
There was a problem hiding this comment.
These should be Int, not Int64
| last_blockindices = Int64.(BlockArrays.blockindex.(bi_stop)) | ||
|
|
||
| orig_blocksizes = blocksizes(A) | ||
| orig_row_sizes = [bs[1] for bs ∈ view(orig_blocksizes, :, 1)] |
There was a problem hiding this comment.
This seems very questionable. I think you might want to be using blockaxes or blocklengths.
I'm not going to review the rest since it all seems overly complicated.
| not guaranteed that the `data` for the result is a contiguous subset of `A.data`. | ||
| """ | ||
| function BlockSkylineMatrix(A::BlockSkylineMatrix{T,DATA,BS}, rows::UnitRange, | ||
| cols::UnitRange) where {T,DATA,BS} |
There was a problem hiding this comment.
You probably want AbstractUnitRange
| `rows` and `columns`. This function allocates new memory and copies data, because it is | ||
| not guaranteed that the `data` for the result is a contiguous subset of `A.data`. | ||
| """ | ||
| function BlockSkylineMatrix(A::BlockSkylineMatrix{T,DATA,BS}, rows::UnitRange, |
There was a problem hiding this comment.
I'm not convinced this is the right name. The comment isn't exactly clear what this doing.
|
Adding tests would make it clear what exactly this is doing. I think this seems overly complicated. I'm pretty sure you can accomplish the same thing in a couple of lines using |
2 participants
Take an existing BlockSkylineMatrix (or BlockBandedMatrix), and select a sub-matrix by passing UnitRanges for the rows and columns to select, returning a BlockSkylineMatrix (or BlockBandedMatrix).