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

Potential issue with `findblock[index]` for `BlockUnitRange` with `first != 1`

Open mtfishman opened this issue 1 year ago • 4 comments

Perhaps I am misunderstanding how findblock[index] is supposed to work, but this behavior is confusing to me:

julia> using BlockArrays

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

julia> findblock(r, 1)
ERROR: BoundsError: attempt to access 2-blocked 5-element BlockedUnitRange{Vector{Int64}} at index [1]
Stacktrace:
 [1] findblock(b::BlockedUnitRange{Vector{Int64}}, k::Int64)
   @ BlockArrays ~/.julia/packages/BlockArrays/L5yjb/src/blockaxis.jl:299
 [2] top-level scope
   @ REPL[27]:1

julia> findblock(r, 2)
Block(1)

julia> findblock(r, 3)
Block(1)

julia> findblock(r, 4)
Block(1)

julia> findblock(r, 5)
Block(2)

I expected it to return:

julia> findblock(r, 1)
Block(1)

julia> findblock(r, 2)
Block(1)

julia> findblock(r, 3)
Block(1)

julia> findblock(r, 4)
Block(2)

julia> findblock(r, 5)
Block(2)

i.e. in a call to findblock(r, k), I was interpreting k as an index, and thought findblock would output the block that index is in, but it seems to be interpreting k as a value, and is outputting the block that value is in.

Maybe the giveaway is that find* functions in Base find the index of a value, so the current behavior of findblock is consistent with that convention, and really I was just hoping for a different function with a different functionality.

mtfishman avatar Mar 19 '24 21:03 mtfishman

I think the current behaviour is correct...

dlfivefifty avatar Mar 20 '24 22:03 dlfivefifty

Yeah I think you are right, I just found it surprising. I think ultimately what I wanted was a function that implemented findblock(only(axes(r)), k), which I think is a more useful concept/functionality.

It seems like for it to be well defined beyond sorted collections with unique elements (like unit ranges), it should be called findfirstblock, but that is another issue.

mtfishman avatar Mar 21 '24 00:03 mtfishman

Call it findaxesblock? For matrices would also support findaxesblock(A, k, j) returning a Block{2}

dlfivefifty avatar Mar 24 '24 12:03 dlfivefifty

findaxesblock seems fine. I'm still not a huge fan of using the naming convention find* for this kind of operation, which is ultimately a conversion from a cartesian index to a block index, which is what inspired my proposal of BlockIndices(a)[k] in #346. Also findaxesblockindex(A, k) gets a bit long, but I suppose so is the interface proposed in #346, where the best I can come up with for the equivalent of findaxesblock(a, k) would be block(BlockIndices(a)[k]), or maybe a slight shorthand block(BlockIndices(a), k) which could skip some of the extra operations needed to get the offset within the block.

mtfishman avatar Mar 25 '24 13:03 mtfishman