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

AbstractBlockPermutation <: AbstractBlockTuple #11

Merged
merged 11 commits into from
Jan 14, 2025

Conversation

ogauthe
Copy link
Contributor

@ogauthe ogauthe commented Jan 13, 2025

This PR refactors AbstractBlockedPermutation to become a subtype of AbstractBlockTuple.

It also generalizes AbstractBlockTuple, which does not assume any type parameter convention.

TBD: refactor AbstractBlockedPermutation type parameters to use BlockLengths,Flat instead of Blocks?

Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 98.21429% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.54%. Comparing base (81818e2) to head (f860cc5).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/blockedpermutation.jl 96.55% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #11      +/-   ##
==========================================
+ Coverage   90.47%   92.54%   +2.07%     
==========================================
  Files          16       16              
  Lines         336      322      -14     
==========================================
- Hits          304      298       -6     
+ Misses         32       24       -8     
Flag Coverage Δ
docs 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@mtfishman
Copy link
Member

TBD: refactor AbstractBlockedPermutation type parameters to use BlockLengths,Flat instead of Blocks?

I think we should just follow the design of AbstractBlockTuple/BlockedTuple, i.e.:

abstract type AbstractBlockPermutation <: AbstractBlockTuple end

struct BlockedPermutation{BlockLengths,Flat} <: AbstractBlockPermutation
  flat::Flat

  function BlockedPermutation{BlockLengths}(flat::Tuple) where {BlockLengths}
    length(flat) != sum(BlockLengths) && throw(DimensionMismatch("Invalid total length"))
    return new{BlockLengths,typeof(flat)}(flat)
  end
end

(also note the rename AbstractBlockedPermutation -> AbstractBlockPermutation.)

@ogauthe
Copy link
Contributor Author

ogauthe commented Jan 13, 2025

I realized TensorAlgebra.contract dispatches on blocklength (and so the current FusionTensor). So I think it makes sens to preserve BlockLength as first type parameter of BlockedPermutation. It makes type parameters redundant, but there is a check that it matches BlockLengths and this should not make any change at runtime.

@mtfishman
Copy link
Member

I realized TensorAlgebra.contract dispatches on blocklength (and so the current FusionTensor). So I think it makes sens to preserve BlockLength as first type parameter of BlockedPermutation. It makes type parameters redundant, but there is a check that it matches BlockLengths and this should not make any change at runtime.

Ah, good point, that sounds like a good plan. I think we should also expose BlockLength as the first type parameter of BlockedTuple as well, since we are often going to want to limit to bipartitition tuples (like in the axes of the FusionTensor), and it would be convenient to type that BlockedTuple{2}. In addition, we should probably expose BlockLength as a type parameter of the abstract types as well.

@ogauthe
Copy link
Contributor Author

ogauthe commented Jan 14, 2025

I think this is ready to merge.

I plan to start using it in FusionTensors.jl. We should split AbstractBlockTuple to a dedicated ITensor Tuple library at some point.

@mtfishman mtfishman changed the title AbstractBlockedPermutation <: AbstractBlockTuple AbstractBlockPermutation <: AbstractBlockTuple Jan 14, 2025
@mtfishman
Copy link
Member

Looks good, thanks! The constructors for BlockedPermutation are definitely a bit of a mess, I think I overcomplicated that a bit, but we can address that in future PRs after we see how it is being used in practice.

@mtfishman mtfishman merged commit b876749 into ITensor:main Jan 14, 2025
11 checks passed
@ogauthe ogauthe deleted the blockedperm branch January 14, 2025 20:44
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