Skip to content
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

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

Open
mtfishman opened this issue Mar 19, 2024 · 4 comments · Fixed by #337
Open

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

mtfishman opened this issue Mar 19, 2024 · 4 comments · Fixed by #337

Comments

@mtfishman
Copy link
Contributor

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
 45
 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
Copy link
Member

I think the current behaviour is correct...

@mtfishman
Copy link
Contributor Author

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
Copy link
Member

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

@mtfishman
Copy link
Contributor Author

mtfishman commented Mar 25, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants