JuliaArrays / BlockArrays.jl

BlockArrays for Julia
http://juliaarrays.github.io/BlockArrays.jl/
Other
192 stars 25 forks source link

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

Open mtfishman opened 3 months ago

mtfishman commented 3 months ago

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.

dlfivefifty commented 3 months ago

I think the current behaviour is correct...

mtfishman commented 3 months ago

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.

dlfivefifty commented 3 months ago

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

mtfishman commented 3 months ago

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.