-
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
Conversation
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
|
@lkdvos @Jutho this is ready for review. The motivation here is both improved code organization and generality (it removes some The main considerations I can think of are:
|
| function left_orth_svd!(A, VC, alg, trunc::Nothing=nothing) | ||
| alg′ = select_algorithm(svd_compact!, A, alg) | ||
| V, C = VC | ||
| S = Diagonal(initialize_output(svd_vals!, A, alg′)) |
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 AbstractMatrix restriction above, but then his part of the implementation (namely the use of Diagonal) is again very much (Abstract)Matrix specific, whereas left_orth_qr! and left_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:
function left_orth_svd!(A, VC, alg, trunc::Nothing=nothing)
alg′ = select_algorithm(svd_compact!, A, alg)
U, S, Vᴴ = svd_compact!(A, alg′)
V, C = VC
return copy!(V, U), mul!(C, S, Vᴴ)
endThere 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 Diagonal with diagonal, and then diagonal could 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 diagview to have a uniform interface to extracting a view of the diagonal of both Matrix and Diagonal, but that could also be specialized for other types. So a type-agnostic construction of a diagonal matrix could also be useful. With diagonal as name, it might get confusing though, as LinearAlgebra.diag is 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_vals can be used to construct the S output of svd_compact. In TensorKit tensors, for example, a DiagonalTensorMap would 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_vals would than rather return that information as a Dict where 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/diagonal work 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.
Jutho
left a comment
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 definitely approve of splitting the different branches into separate functions that can be further specialized upon, and in omitting the AbstractMatrix restriction where possible. But as mentioned in the comments, it would still be good to add this restriction in the methods that use specific matrix constructions such as Diagonal.
Jutho
left a comment
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 think this is ready? I have no further comments.
Co-authored-by: Jutho <[email protected]> Co-authored-by: Lukas Devos <[email protected]>
As suggested by @lkdvos. Requires #23.