Conversation
|
Oh man this will make life a lot easier for the GPU support, thanks for doing this! |
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| :iterative => | ||
| (; tol = 1.0e-14, krylovdim = 25, kwargs...) -> | ||
| IterSVD(; alg = GKL(; tol, krylovdim), kwargs...), | ||
| :divideandconquer => LAPACK_DivideAndConquer, |
There was a problem hiding this comment.
this is a very breaking change, are we certain that we want to rename the symbols?
I have no specific preference, as I am not really affected, but I also don't necessarily see the benefit of the rename, so just wanted to double-check
There was a problem hiding this comment.
Also, can we not force these to be LAPACK by default?
There was a problem hiding this comment.
Could we instead do what MPSKit does and allow a settable default?
There was a problem hiding this comment.
I'm not entirely sure what you mean here, this is just a list of "short-hand-symbols" to avoid having to type full names, I don't really think there is a way to make this "settable" ?
There was a problem hiding this comment.
I kind of did this on a whim but I do think this is much more consistent overall. That's also the way the eigh algorithms are named. Of course this is breaking but I would hope that it wouldn't mess up too many peoples code because mostly they should have used the default SVD algorithm anyway. So I'd be in favor of keeping this change but if there is a strong case to be made for keeping :sdd, :svd and :iterative then that's fine by me.
There was a problem hiding this comment.
I think my main argument against is that at that point it's not really that clear to me why we even have these symbols, and why not simply use LAPACK_DivideAndConquer in its entirety?
There was a problem hiding this comment.
While I do understand that argument, I did shorten the names for super long algorithms names, e.g. for :multiple. So most of the symbols are not that long. For :divideandconquer I kept it that way because its a memorable term that people usually know.
|
Once tests are up and running, I would really like to triple check the actual accuracy of the gradients to make sure there's no regressions (and then also add this test to CI, don't know why I didn't do that before). Just to avoid running into something like #276 later. |
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
|
Would be good to go for me once the tests turn green |
4 participants
So far we defined a custom SVD pullback inside of PEPSKit which copied over some old TensorKit code. Here I remove that old part and instead use MatrixAlgebraKit's
svd_pullback!andsvd_trunc_pullback!. This makes all the SVD routines also more consistent with the newly addedeighforward and pullback code.As a side note: What initially held me back from immediately using MAK's pullbacks was that it didn't implement any Lorentzian broadening. However, it does use a
safe_invwith a cutoff tolerance which should in principle work just as well. So here I decided to completely scrap the Lorentzian broadening code and only usesave_invs via MAK's pullbacks. To be able to adjust the degeneracy/broadening tolerance, I added adegeneracy_tolto all the pullback structs.