-
Notifications
You must be signed in to change notification settings - Fork 5
Make orthnull more customizable and general #25
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
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
e53f0c3
Make orthnull more customizable
mtfishman 4448b07
Fix typo
mtfishman b710816
Merge branch 'main' into mf/orthnull_customization
mtfishman b80118a
Fix some tests
mtfishman 7cc51bf
Properly forward truncation to right_null
mtfishman c030ce5
orth for non-AbstractMatrix
mtfishman 8887591
More robust non-AbstractMatrix test
mtfishman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to omitting the
AbstractMatrixrestriction above, but then his part of the implementation (namely the use ofDiagonal) is again very much(Abstract)Matrixspecific, whereasleft_orth_qr!andleft_orth_polar!are quite generic and probably work for other types as well.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a more general implementation can be added that is less economic with its memory usage:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I wasn't being careful about the type restriction in this case. I think that's a good suggestion to have a more generic version.
Another thing I can think of is replacing
Diagonalwithdiagonal, and thendiagonalcould be an interface function that types can overload to construct a diagonal matrix-like object. But I like your suggestion better.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, internally there is already the
diagviewto have a uniform interface to extracting a view of the diagonal of bothMatrixandDiagonal, but that could also be specialized for other types. So a type-agnostic construction of adiagonalmatrix could also be useful. Withdiagonalas name, it might get confusing though, asLinearAlgebra.diagis exactly the opposite, namely extracting the diagonal of a matrix and returning it as a vector. But I wouldn't have another naming suggestion.I am also not sure if we generally want that the output type of
svd_valscan be used to construct theSoutput ofsvd_compact. In TensorKit tensors, for example, aDiagonalTensorMapwould still store all the singular values in a single list (Vector), but with internal structure such that it is known which parts of this vector are associated with which sectors/quantum numbers.svd_valswould than rather return that information as aDictwhere for every sector (being the keys into the dict) there is a separate vector with only the singular values for that sector.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it definitely would require some work to make the design based on
diagview/diagonalwork for exotic types like that.I've updated the code to add a non-AbstractMatrix codepath based on your suggestion above. I also added a test that defines a simple non-AbstractMatrix to test that codepath.