CompatHelper: bump compat for TensorKit to 0.15#270
Conversation
b32ae69 to
343f0e2
Compare
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
|
I adopted most of the easy to the new TensorKit/MatrixAlgebraKit interface and adjusted some of the naming as well. Regarding the naming there are some questions:
In both case I don't have strong feelings and would be fine with leaving them as they are. The main thing that's left to be done is the mess inside of One last thing I noticed was that MatrixAlgebraKit actually gauge-fixes the SVDs in some way. I was wondering if this would actually lead to element-wise convergence even without our more sophisticated gauge fixing routine. From what I heard, some PEPS practicioners actually fix their CTMRG by just fixing the SVD to reach element-wise convergence. (I do think this is an inherently worse idea and has no general guarantee to work but it would be significantly cheaper.) |
|
I'm not sure that the SVD gauge fixing is actually sufficient: the issue remains that there are gauge degrees of freedom that you can propagate "around the ring" of environments. |
Co-authored-by: Lukas Devos <ldevos98@gmail.com>
… update with new MAK functions
|
This should work now (up to small things in the tests - let's see if the CI runs through). I left most of the things on the user side untouched and tried to contain the messiness inside of the SVD wrapper interface. This way we won't have to break things in the future (hopefully). There we will have to update things to properly interface with MatrixAlgebraKit's pullbacks once we can do broadening. Also we should remember that currently all truncation errors are being set to zero; so that has to be updated once TensorKit implements that. In any case, a more involved revamping of the PEPSKit-TensorKit-KrylovKit SVD trinity should anyway happen in a separate PR with more intention. For now this seems usable and I wouldn't want to wait much longer to make PEPSKit compatible with TensorKit v0.15. |
|
Update: We'll have to wait until TensorKit tags a new release since the |
|
Also something regarding Apparently there is a new assertion in function truncspace(space::ElementarySpace; by = abs, rev::Bool = true)
isdual(space) && throw(ArgumentError("resulting vector space is never dual"))
return TruncationSpace(space, by, rev)
endwhich wasn't there before. I'm wondering what's going wrong here since the inputs didn't change as far as I can tell. If someone has an idea, let me know. Otherwise, I'll take a look again on Monday. |
|
I think this was a silent bug previously (which is why I added the assertion), also associated to how the decompositions were being handled. When we performing an SVD, you can always make the canonical choice of In some sense, this means that the choice of arrows on our states and environments is actually dictated partially by the algorithms we are performing on them, e.g. even if you start with dual spaces, an algorithm that performs SVDs could alter this. |
Yes, during the backpropagation Zygote tries to accumulate all of the adjoints for the corners and edges by putting them in a The easiest fix would probably be to exclude |
Actually I agree. Then I'll rename all occurences of
I think here I would be fine with just leaving |
How do you feel about something like this: struct CTMRGEnv{...}
...
function CTMRGEnv{}(...)
foreach(check_ctmrg_spaces, corners)
foreach(check_ctmrg_spaces, corners)
new{}(...)
end
end
check_ctmrg_spaces(x::AbstractZero) = nothing
check_ctmrg_spaces(x::CTMRG_EdgeTensor) = ...
...I think that's both idiomatic, extensible, and not too hacky (naming up for grabs as usual)
I'm definitely in favor, will do the same for MPSKit in the next set of breaking changes I think.
I think these symbols are fine, although in the long run it would be nice to see if we can simply always use |
|
Zygote was crashing on the So it's not really a problem here, but it seems that |
|
Only one failing test remaining. It seems that there's a CTMRG that completely fails to converge in the test for the Fermi-Hubbard model with 2-site and 3-site simple update, causing the test of the 2-site simple update energy to fail (but only in one instance for some reason). Maybe we can try growing from a smaller initial environment that doesn't have the full truncation rank yet (maybe even from a product state) to stabilize the contraction? |
…version/2025-10-04-01-12-41-218-01493734210
|
To converge the CTMRGEnv for the 2/3-site Fermi-Hubbard SU test, I now use the SUWeight as initialization. Thing should now work for all platforms. |
|
This would be good to go for me now (if the tests pass) :-) |
|
Let me go over this in a bit more detail today. I'm slightly unhappy about not being able to further simplify the svd parts, but I guess it is better to first get this merged and working again, and then go in and try to simplify this further. Anyways, review incoming today |
lkdvos
left a comment
There was a problem hiding this comment.
Left some final small comments, otherwise good for me!
This pull request changes the compat entry for the
TensorKitpackage from0.14.9to,0.14.90.15.This keeps the compat entries for earlier versions.Note: I have not tested your package with this new compat entry.
It is your responsibility to make sure that your package tests pass before you merge this pull request.