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

Add findblockindex support for AbstractArray #345

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

putianyi889
Copy link
Contributor

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

@putianyi889 putianyi889 changed the title Add findblockindex support for AbstractBlockArray Add findblockindex support for AbstractArray Mar 21, 2024
Copy link

codecov bot commented Mar 21, 2024

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 ⚠️
src/abstractblockarray.jl 0.00% 3 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jishnub
Copy link
Member

jishnub commented Mar 21, 2024

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

@putianyi889
Copy link
Contributor Author

to not use Base.IteratorsMD.flatten

Do you have an alternative that concats tuples?

@jishnub
Copy link
Member

jishnub commented Mar 21, 2024

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

@mtfishman
Copy link
Collaborator

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
 34
 5
 6

julia> a2 = mortar([2:3, 4:6])
2-blocked 5-element BlockVector{Int64, Vector{UnitRange{Int64}}, Tuple{BlockedUnitRange{Vector{Int64}}}}:
 2
 34
 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
Copy link
Collaborator

mtfishman commented Mar 22, 2024

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).

@putianyi889
Copy link
Contributor Author

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 putianyi889 reopened this Mar 31, 2024
@putianyi889 putianyi889 marked this pull request as draft March 31, 2024 14:35
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 this pull request may close these issues.

3 participants