Skip to content

[WIP] MatrixAlgebraKit decompositions #230

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

Open
wants to merge 48 commits into
base: master
Choose a base branch
from
Open

[WIP] MatrixAlgebraKit decompositions #230

wants to merge 48 commits into from

Conversation

lkdvos
Copy link
Collaborator

@lkdvos lkdvos commented Mar 16, 2025

This PR is part of a large set of changes that alters the backend from TensorKit.MatrixAlgebra to MatrixAlgebraKit for the factorizations.
Concretely, this is achieved through a new submodule TensorKit.Factorizations that implements most of the MatrixAlgebraKit interface, along with some overloads and reimplementations to be somewhat backwards compatible.

New features

  • foreachblock(f, t::AbstractTensorMap...) is a new function that is meant to provide a uniform interface to iterate through the blocks of a collection of tensors. The main purpose is to centralize this concept to easily add multithreading over the blocks. In order to break up this PR the actual multithreading implementation is left for a follow-up PR.
  • eig, eigh and eigen now have preliminary support for a truncated version of these algorithms.
  • The factorizations can now swap out their backends to select alternate algorithms or implementations.
  • ominus and its unicode variant can now be used to obtain the orthogonal complement of a space, ie if W = oplus(V1, V2), V2 = ominus(W, V1).

Breaking changes

  • The tsvd function no longer outputs the truncation error, and instead just gives U, S, Vh.
  • The truncation strategies are now directly inherited from MatrixAlgebraKit. This means truncdim is replaced by truncrank, trunctol is replaced by truncbelow, and there are deprecation warnings for the old functions.
  • The leftorth and rightorth functions will now always output tensors with a single space connecting them, which was not the case for Polar decompositions previously. To retrieve the old behavior with isposdef R factors (equal domain and codomain instead of isomorphic), new functions leftpolar and rightpolar are added.
  • The leftorth and rightorth functions will now always have a connecting space with isdual=false. This is different from the previous behavior where some N,1 or 1,N tensors kept the isdual flag on the connecting leg.

@lkdvos lkdvos requested a review from Jutho March 16, 2025 20:19
@lkdvos lkdvos marked this pull request as draft March 16, 2025 20:19
Copy link

codecov bot commented Mar 16, 2025

Codecov Report

Attention: Patch coverage is 63.93897% with 260 lines in your changes missing coverage. Please review.

Project coverage is 77.63%. Comparing base (0c73bd2) to head (ac26a7e).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/tensors/factorizations/matrixalgebrakit.jl 67.15% 89 Missing ⚠️
src/tensors/factorizations/truncation.jl 53.33% 70 Missing ⚠️
src/tensors/factorizations/factorizations.jl 34.17% 52 Missing ⚠️
src/tensors/diagonal.jl 0.00% 18 Missing ⚠️
src/tensors/factorizations/implementations.jl 84.90% 16 Missing ⚠️
src/tensors/backends.jl 0.00% 8 Missing ⚠️
ext/TensorKitChainRulesCoreExt/factorizations.jl 92.10% 3 Missing ⚠️
src/spaces/cartesianspace.jl 0.00% 3 Missing ⚠️
src/spaces/vectorspaces.jl 0.00% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0c73bd2) and HEAD (ac26a7e). Click for more details.

HEAD has 6 uploads less than BASE
Flag BASE (0c73bd2) HEAD (ac26a7e)
12 6
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   82.65%   77.63%   -5.03%     
==========================================
  Files          43       48       +5     
  Lines        5593     5597       +4     
==========================================
- Hits         4623     4345     -278     
- Misses        970     1252     +282     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lkdvos
Copy link
Collaborator Author

lkdvos commented Jun 12, 2025

The test failures on empty tensors should be resolved once QuantumKitHub/MatrixAlgebraKit.jl#37 is merged.

@Jutho I think this is ready for another round of review, it definitely requires a bunch more cleanup but I think I am getting somewhere now, the improvements in MatrixAlgebraKit have definitely helped a bunch.
I'm struggling quite a bit with figuring out how breaking of a change I would be okay with, but making sure all old algorithms and syntaxes still work is turning out to be a major pain.
I'm not so sure if it is even worth it to keep QR, SVD, ... around, and while I could make them type aliases for their MatrixAlgebraKit counterparts in the meantime, this doesn't work for QRpos. I am leaning towards just doing that though, since it would definitely simplify more code.

@@ -265,6 +275,21 @@ function LinearAlgebra.mul!(dC::DiagonalTensorMap,
return dC
end

function LinearAlgebra.lmul!(D::DiagonalTensorMap, t::AbstractTensorMap)
domain(D) == codomain(t) || throw(SpaceMismatch())
for (c, b) in blocks(t)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we immediately want to write this using foreachblock?

end
function LinearAlgebra.rmul!(t::AbstractTensorMap, D::DiagonalTensorMap)
codomain(D) == domain(t) || throw(SpaceMismatch())
for (c, b) in blocks(t)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment/question.

@lkdvos lkdvos force-pushed the matrixalgebra branch 4 times, most recently from dc34c24 to 1903b01 Compare June 16, 2025 15:15
@lkdvos lkdvos marked this pull request as ready for review June 17, 2025 01:42
⊖(V::ElementarySpace, W::ElementarySpace) -> X::ElementarySpace
ominus(V::ElementarySpace, W::ElementarySpace) -> X::ElementarySpace

Return the set difference of two elementary spaces, i.e. an instance `X::ElementarySpace`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Return the set difference of two elementary spaces, i.e. an instance `X::ElementarySpace`
Return a space that is equivalent to the orthogonal complement of `W` in `V`, i.e. an instance `X::ElementarySpace`

export leftorth, rightorth, leftnull, rightnull,
leftorth!, rightorth!, leftnull!, rightnull!,
export leftorth, rightorth, leftnull, rightnull, leftpolar, rightpolar,
leftorth!, rightorth!, leftnull!, rightnull!, leftpolar!, rightpolar!,
Copy link
Owner

@Jutho Jutho Jul 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add new methods like leftpolar here, or do we want to switch over to MatrixAlgebraKit conventions and use left_ and right_ (with underscores), and keep leftorth and friends for backward compatibility (with possible deprecations in a future version)?


for f! in (:svd_compact!, :svd_full!, :left_null_svd!, :right_null_svd!)
@eval function select_algorithm(::typeof($f!), t::T, alg::SVD;
kwargs...) where {T}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this for general types T?

end

function check_input(::typeof(right_polar!), t::AbstractTensorMap, (P, Wᴴ)::_T_PWᴴ)
domain(t) ≿ codomain(t) ||
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific preference for writing the checks using versus ?

I know this is nitpicking, but I would either prefer consistent use of , or alternatively, consistent use of codomain(t) as first argument and domain(t) as second, with varying operator.

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