BlockArrays.jl icon indicating copy to clipboard operation
BlockArrays.jl copied to clipboard

Add `findblockindex` support for `AbstractArray`

Open putianyi889 opened this issue 1 year ago • 7 comments

If you agree with this idea then I'll add tests.

putianyi889 avatar Mar 21 '24 11:03 putianyi889

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 95.60%. Comparing base (5a72c40) to head (a34ba1f).

Files Patch % Lines
src/blockaxis.jl 0.00% 8 Missing :warning:
src/abstractblockarray.jl 0.00% 3 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #345      +/-   ##
==========================================
- Coverage   96.25%   95.60%   -0.65%     
==========================================
  Files          18       18              
  Lines        1628     1639      +11     
==========================================
  Hits         1567     1567              
- Misses         61       72      +11     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 21 '24 11:03 codecov[bot]

Seems good to me, but it'll be better to not use Base.IteratorsMD.flatten, which is an internal function.

jishnub avatar Mar 21 '24 12:03 jishnub

to not use Base.IteratorsMD.flatten

Do you have an alternative that concats tuples?

putianyi889 avatar Mar 21 '24 12:03 putianyi889

Perhaps foldl((x,y) -> (x...,y...), tuple_of_tuples)?

jishnub avatar Mar 21 '24 13:03 jishnub

This PR actually exemplifies the point I was trying to make in #336. It leads to this inconsistency:

julia> using BlockArrays

julia> a1 = BlockArrays._BlockedUnitRange(2, [3, 6])
2-blocked 5-element BlockedUnitRange{Vector{Int64}}:
 2
 3
 ─
 4
 5
 6

julia> a2 = mortar([2:3, 4:6])
2-blocked 5-element BlockVector{Int64, Vector{UnitRange{Int64}}, Tuple{BlockedUnitRange{Vector{Int64}}}}:
 2
 3
 ─
 4
 5
 6

julia> findblockindex(a1, 4)
Block(2)[1]

julia> findblockindex(a1, (4,))
Block(2)[2]

julia> findblockindex(a2, (4,))
Block(2)[2]

i.e. it is not clear if findblock[index](a, k) is supposed to treat k as an index into the collection or a value of the collection, and there seems to be an inconsistency depending on what is input.

mtfishman avatar Mar 22 '24 12:03 mtfishman

Another proposal for a "spelling" of this kind of operation (converting from a cartesian index to a block index) would be #346.

With that proposal, instead of using findblockindex(a1, (4,)) == Block(2)[2], you would use BlockIndices(a1)[4] == Block(2)[2]. I don't think the functionality of converting from a cartesian index to a block index should be called find*, since find*(a, k) in Base refers to finding the index of a value k in a collection a (those only happen to correspond if the collection is a unit range starting at 1).

mtfishman avatar Mar 22 '24 12:03 mtfishman

It's the name of findblockindex that is confusing. This PR is intended to implement to_blockindex instead, which is analogue to to_indices. to_blockindices is a better analogy but I haven't considered how that will work.

putianyi889 avatar Mar 31 '24 14:03 putianyi889