Refactor orthogonalization and nullspace interface #79
Conversation
|
As a sidenote: there are some errors right now because we first |
lkdvos
changed the title
|
So, I've updated the docs and syntaxes and docstrings etc, I think this PR should now really be ready for its final round of review. |
|
After the build completes, the updated documentation will be available here |
lkdvos
added
enhancement
|
Everything looks good to me besides those small comments I left. |
| return Vᴴtrunc, ind | ||
| end | ||
|
|
||
| # disambiguate: |
There was a problem hiding this comment.
Are the methods below ambiguous with the methods above? They seem strictly more specific. Or are they ambiguous with methods that specify ::NoTruncation but not TU<:ROCMatrix?
There was a problem hiding this comment.
The latter, I special-cased ::NoTruncation for the *_null implementations and this is ambiguous with the overall special-cased truncate defined above. I think in general these GPU methods still require scalar indexing, so it's not actually fully supported yet anyways, so we will probably revisit this and update that in the near future as well, but that doesn't have to be tackled in this PR
|
Thanks @lkdvos, definitely a big improvement on this part of the code that will be easier to overload and extend externally. |
* Bump v0.6 * rename `gaugefix` -> `fixgauge` * reduce unnecessary warnings * fix `copy_input` signatures in Mooncake tests * Add changelog to docs
This is an alternative suggestion for #77, based on discussions with @mtfishman .
Here, instead of using
left_orth_qrandleft_orth_svd, we use a wrapper algorithm type to do the dispatching.I've partially implemented this for the
left_orthandright_orthcases, there are still some parts that would need changes to fully go to this new interface.I'm not entirely sure which one I like best. Here the advantage would be that in principle we don't need additional functions, and do the dispatching based on the algorithm type alone. It doesn't really provide a solution to being able to call
left_orth!(A; alg = LAPACK_HouseholderQR()by itself, since that would still need some trait or rewrapping in order to fully work, but it does circumvent the issue by allowing you to simplyleft_orth!(A; alg = LeftOrthViaQR(LAPACK_HouseholderQR())instead.