DMRG3S: strictly single-site DMRG subspace expansion (Hubig et al. 2015)#460
DMRG3S: strictly single-site DMRG subspace expansion (Hubig et al. 2015)#460VinceNeede wants to merge 19 commits into
Conversation
|
Your PR no longer requires formatting changes. Thank you for your contribution! |
Codecov Report❌ Patch coverage is
... and 79 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Thanks for this, I had a brief look and I think it already looks quite well. Considering the expansion algorithm itself - without having looked in a lot of detail so definitely feel free to tell me I'm wrong - I think it could be nice to avoid having both an For the TDVP implementation, I definitely agree to flush this one out first, and honestly it's probably easier to tackle and review that in a separate PR. Considering your other question, I'm definitely happy to add the |
776835c to
f2c34cc
Compare
|
Hi @lkdvos, Pushed a sketch of merging I haven't wired up Since |
lkdvos
left a comment
There was a problem hiding this comment.
Thanks for rebasing onto that other work!
Considering the DMRG constructor, I think I agree that we should exclude being able to set conflicting schemes, but unless I'm missing something that would already not be possible - the gauging algorithm is either a (truncated) orth algorithm or DMRG3S, and in the latter case there is only a single (truncated) orth algorithm in the DMRG3S.
If at all possible, I would like to design the DMRG constructor to be able to take the trscheme still, and have that routed through automatically, or error if both a trscheme and DMRG3S are provided. My experience is that while these "algorithm structs" are very convenient as a developer, it is somewhat harder as a newcomer to the package to keep track of everything, and a simple keyword-argument approach, which possibly does not expose all settings but handles the common cases, helps alleviate some of these issues.
Update
TODO
|
|
Thanks for the update! I commented on the reuse of the AC_projection, hopefully that makes sense, and I think it is safe to assume that the alg_gauge computation per site is not hurting performance, indeed after these two this looks ready to go for me. |
|
Both two last points have been implemented. I have explicitly update the compatibility of vince@fedora:~/MPSKit.jl$ git log --oneline -1
660b14e (HEAD -> vb-dmrg3s, origin/vb-dmrg3s) add _get_combiner function and update gauge! to use MPO_AC_Hamiltonian for perturbation calculation
vince@fedora:~/MPSKit.jl$ git runic main
runic did not modify any files |
There was a problem hiding this comment.
I left some minor things to resolve, will merge afterwards:
- the
gauge!function is slightly inconsistent in its argument order, I think currently bothpsias first and third argument exists, could you make that the same order everywhere? - Can we still simplify by getting rid of the
NoExpandalgorithm wrapper?
lkdvos
left a comment
There was a problem hiding this comment.
Thanks for all the work, will merge as soon as tests pass!
|
Thanks for the review! I've implemented the last two points we discussed. I also ran the formatter locally since it's still blocked here, let me know if the CI flags anything I missed, or if you'd like any further changes. Really want to say thanks for the time and care you put into this review. I learned a lot about the internals of MPSKit going through it with you, and I appreciate you taking a somewhat unconventional PR from someone new to the codebase this seriously. |
Add `FunctionalSchedule`, wrapping an arbitrary `(noise, iter, ϵ) -> noise` callable as a `NoiseSchedule`. Overload `∘` on `NoiseSchedule` to compose two schedules into a `FunctionalSchedule`, following the usual function composition convention (`s2` applied first, `s1` applied to its result). Add a `threshold` keyword to `ExponentialDecay`, snapping the decayed noise to exactly zero once it falls below it -- avoids running the (cheap but non-free) expansion step indefinitely on a vanishingly small amplitude. This also makes `ExponentialDecay ∘ Warmup` a natural way to combine a hard iteration cutoff with smooth decay.
fix typo in variable name
…mic `MPO_AC_Hamiltonian`
…n for perturbation calculation
|
Hopefully now the format tests are fixed as well, will merge as soon as we get the green light from the tests :) |
Summary
Implements DMRG3S — strictly single-site DMRG with subspace expansion — from Hubig, McCulloch, Schollwöck & Wolf, Phys. Rev. B 91, 155115 (2015). Unlike CBE (
alg_expand), which enriches the bond before the local eigensolve, DMRG3S enriches it after, using a perturbation built from the just-optimized tensor, the local Hamiltonian, and the environment. This lets single-site DMRG escape local minima caused by missing quantum-number sectors that CBE-style pre-expansion can't reach.This PR also targets TDVP, pending design agreement
DMRG only for now. TDVP support belongs in this same PR, but I'd like design agreement on the interface below first, since it directly shapes how it plugs into TDVP too. Docs are also on hold for the same reason — no point documenting an interface that might still change.
What's here
find_groundstate!'s left-to-right and right-to-left sweeps from two separate, near-duplicate loops into a single loop overdirection in (:right, :left), dispatching onVal. No behavioral change; this just made room for the new expansion step without doubling its code.alg_post_expandfield onDMRG, dispatching a newpost_expand!step after the eigensolve, alongside the existing pre-eigensolvealg_expand. Default (NoExpand()) reproduces current behavior exactly.DMRG3S, with a composableNoiseSchedulefor the mixing factor (ExponentialDecay,Warmup,∘for combining them).alg_expandandalg_post_expandare independent and can be used together or separately.alg_gauge, since the bond would otherwise grow unboundedly each sweep.Open design questions for maintainers
alg_post_expandsentinel: I used a dedicatedNoExpand()type rather thannothing(whichalg_expanduses). This letsNoExpandcarry its ownpost_expand!method for dispatch, but it's an inconsistency withalg_expand's convention worth flagging.post_expand!call rather than kept separate, because splitting them left the MPS briefly inconsistent between steps (caught via a real bug during development — happy to share details). This does meanpost_expand!needsalg_gaugethreaded in as a parameter, coupling it to the gauge step more tightly thanalg_expandis.TODO before merge