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

[BlockSparseArrays] Redesign nested views #1504

Merged
merged 27 commits into from
Jun 25, 2024

Conversation

mtfishman
Copy link
Member

@mtfishman mtfishman commented Jun 19, 2024

To-do:

  • Fix tests marked as @test_broken.

Currently, when taking views of views of BlockSparseArrays, the result is sometimes nested views:

julia> using BlockArrays: Block

julia> using NDTensors.BlockSparseArrays: BlockSparseArray

julia> a = BlockSparseArray{Float64}([2, 3], [2, 3])
typeof(axes) = Tuple{BlockArrays.BlockedOneTo{Int64, Vector{Int64}}, BlockArrays.BlockedOneTo{Int64, Vector{Int64}}}

Warning: To temporarily circumvent a bug in printing BlockSparseArrays with mixtures of dual and non-dual axes, the types of the dual axes printed below might not be accurate. The types printed above this message are the correct ones.

2×2-blocked 5×5 BlockSparseArray{Float64, 2, Matrix{Float64}, NDTensors.SparseArrayDOKs.SparseArrayDOK{Matrix{Float64}, 2, NDTensors.BlockSparseArrays.BlockZero{Tuple{BlockArrays.BlockedOneTo{Int64, Vector{Int64}}, BlockArrays.BlockedOneTo{Int64, Vector{Int64}}}}}, Tuple{BlockArrays.BlockedOneTo{Int64, Vector{Int64}}, BlockArrays.BlockedOneTo{Int64, Vector{Int64}}}}:
 0.0  0.00.0  0.0  0.0
 0.0  0.00.0  0.0  0.0
 ──────────┼───────────────
 0.0  0.00.0  0.0  0.0
 0.0  0.00.0  0.0  0.0
 0.0  0.00.0  0.0  0.0

julia> @views a[[Block(2), Block(1)], [Block(2), Block(1)]][Block(1, 1)]
3×3 view(view(::BlockSparseArray{Float64, 2, Matrix{Float64}, NDTensors.SparseArrayDOKs.SparseArrayDOK{Matrix{Float64}, 2, NDTensors.BlockSparseArrays.BlockZero{Tuple{BlockArrays.BlockedOneTo{Int64, Vector{Int64}}, BlockArrays.BlockedOneTo{Int64, Vector{Int64}}}}}, Tuple{BlockArrays.BlockedOneTo{Int64, Vector{Int64}}, BlockArrays.BlockedOneTo{Int64, Vector{Int64}}}}, [3, 4, 5, 1, 2], [3, 4, 5, 1, 2]), BlockSlice(Block(1),1:3), BlockSlice(Block(1),1:3)) with eltype Float64:
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

These nested views can become challenging to deal with. The goal of this PR will be to flatten them down to a single view.

The goal is basically to make use of more of the generic view logic in Base Julia (i.e. in https://github.com/JuliaLang/julia/blob/master/base/subarray.jl), which flattens views using a function reindex. The primary customization point for how reindex behaves is by overloading the to_indices function. BlockArrays.jl has a lot of useful to_indices definitions for Block and other related types in https://github.com/JuliaArrays/BlockArrays.jl/blob/master/src/views.jl, but some are missing for operations we want, so the goal here is to write a minimal number of to_indices overloads to complement the ones already in Base or BlockArrays.

This is a follow-up to #1503 but will provide a more systematic fix to those kinds of issues. Additionally, the goal is to fix a few of slicing bugs listed in ITensor/BlockSparseArrays.jl#2, and also those marked as broken in the BlockSparseArrays tests: https://github.com/ITensor/ITensors.jl/blob/v0.6.14/NDTensors/src/lib/BlockSparseArrays/test/test_basics.jl.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.78%. Comparing base (d3afdb7) to head (078105c).

Current head 078105c differs from pull request most recent head 4873cab

Please upload reports for the commit 4873cab to get more accurate results.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (d3afdb7) and HEAD (078105c). Click for more details.

HEAD has 3 uploads less than BASE | Flag | BASE (d3afdb7) | HEAD (078105c) | |------|------|------| ||4|1|
Additional details and impacted files
@@             Coverage Diff             @@
##             main    ITensor/ITensors.jl#1504       +/-   ##
===========================================
- Coverage   78.06%   60.78%   -17.29%     
===========================================
  Files         148      148               
  Lines        9655     9646        -9     
===========================================
- Hits         7537     5863     -1674     
- Misses       2118     3783     +1665     

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

@mtfishman mtfishman changed the title [WIP] [BlockSparseArrays] Redesign nested views [BlockSparseArrays] Redesign nested views Jun 25, 2024
@mtfishman mtfishman marked this pull request as ready for review June 25, 2024 18:25
@mtfishman mtfishman merged commit ab8a59e into main Jun 25, 2024
15 of 16 checks passed
@mtfishman mtfishman deleted the BlockSparseArray_redesign_nested_slicing branch June 25, 2024 19:14
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.

2 participants