Bypass some overhead in TensorMap constructor, bypass overhead in sectorequal/sectorhash for trivial symmetry#476
Conversation
|
I tested an earlier version of this branch (commit Allocation counts for DMRG dropped ~25-30% consistently:
( More broadly, essentially every MPO-MPS-application benchmark across all three algorithms and both tested sizes shows a real improvement — time ratios mostly 0.67–0.99, memory ratios 0.39–1.00, with zero memory regressions anywhere in the suite. DMRG's own time ratios land in a similar 0.88–0.99 range. Happy to share the full |
|
That is great to hear. Can I ask why you want to create a new MPS/MPO package, instead of using MPSKit, or, if you find that functionality is missing, contributing to it? I see in the description of your package that you think that MPSKit is somehow "VUMPS first", but that doesn't make any sense. MPSKit has full support for standard finite MPS, including DMRG, TDVP and a bunch of other functionality (like finite size excitations). |
|
Also, @VinceNeede , if you are interested in a "finite MPS first" package, there is also https://github.com/Qiaoyi-Li/FiniteMPS.jl . |
|
Happy to hear the changes are helping in any case. I'm running our own benchmark suite locally as well and will get back with the results. A couple remarks I still have though: looking at just the allocation number is often a misleading metric for the actual performance - the allocations we are saving here are of the order of bytes, not the actual kilo or megabytes for the data of the tensors themselves. While it is true that this puts some pressure on the garbage collector, realistically tthis should be subdominant in most tensor network applications. The fact that your timings are affected by this change so drastically might be something worth looking into - I of course don't have much detail about what you are running, but it seems like if you are getting speedups of ~30%, that means more than ~30% of your runtime is spent in constructing tensors. Of course, a lot of time, effort and expertise has already gone in these packages that Jutho mentioned, so I would also strongly advise to always consider contributing first, before feeding all this effort of other people into an LLM and simply building your own copy. |
Codecov Report❌ Patch coverage is
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hi @Jutho, thanks for reaching out. While this PR might not be the best place for a long architectural discussion, I'd love to clear up the context. This honestly started as a personal passion project to learn the ropes, even before I was fully aware of TensorKit. When I briefly looked at MPSKit, I noticed the absence of direct MPO-MPS contraction algorithms (like zip-up or density matrix methods) and found its handling of orthogonality quite complex compared to what I was used to in frameworks like ITensor. This led me to assume the package simply had a different scope from what I wanted to build. Because I was largely driven by the personal satisfaction of building something from scratch, my evaluation of MPSKit was admittedly superficial, and I completely take your point regarding its robust finite-size support. My "VUMPS-first" characterization was definitely off-base. That said, I'd much rather collaborate than duplicate effort. If you think any of the features I'm working on—like zip-up/density matrix contractions, successive randomized compression, DMRG3S for single-site subspace expansion, or future open quantum system algorithms—would be valuable additions to MPSKit, I would be incredibly glad to contribute them! |
This removes the checks in the inner constructors to allow TensorMap construction without any overhead. To keep some of the warnings, I've added this back to some of the outer constructors.
Co-authored-by: Jutho <Jutho@users.noreply.github.com>
|
Hi @lkdvos, Thanks for the feedback. To clarify the timings, the significant speedup I observed was specifically at lower bond dimensions, while higher bond dimensions were almost unaffected. The high allocation count was what prompted me to do a finer profiling in the first place, as I was worried something was off. Regarding your other point, my intention was never to disrespect or copy anyone's hard work. While I have experience with MPS from my master's thesis, I didn't feel confident enough to contribute directly to an advanced ecosystem like TensorKit/MPSKit initially, which is why I started a personal project to learn. I use an LLM purely as an interactive pair-programmer executing under my own direction—for brainstorming architectural ideas, debugging specific edge-case failures, and handling maintenance tasks like writing tests and benchmarks—not to copy existing codebases; in fact, since some algorithms I am focusing on (like DMRG3S and Successive Randomized Compression) aren't currently available out-of-the-box in the major packages, I am implementing them directly from the papers. I'm glad the issue I opened helped improve things here. As I mentioned to Jutho, I would much rather collaborate than duplicate effort, and I'd be very glad to contribute any of the algorithms I'm working on to MPSKit if they fit your roadmap. |
Contraction benchmarks:
|
| Group | Cases | Median (dirty/main) |
|---|---|---|
| mera | 24 | 0.99 |
| mpo | 32 | 1.00 |
| pepo | 32 | 0.99 |
In general I think this confirms the claim that this does not show up for most larger tests :)
Concerning the MPSKit features, I am always happy to have more contributors and to include more of the missing functionality. The goal of that package and this ecosystem in general has always been to be a collaborative experience, so if you feel up for it I will gladly invest time discussing and going over ideas, features and PRs, as well as answer any questions you might have.
|
That matches what I'd been seeing too @lkdvos — reassuring to see it hold up on a broader test suite. Really appreciate the offer to invest time in this — I'd like to take you up on it. I have a couple of specific things I want to work through (one about CBE, one about whether/how SRC could fit given its current limitations) — where would be the best place to continue, a GitHub Discussion, an issue, or somewhere else? |
|
My email is on my github profile, if you'd like to schedule an online meeting I'm happy to discuss! (I'm in ET time zone) |
|
Thanks, @lkdvos — I'll email you shortly to set up a time! |
3 participants
As discussed with @Jutho and @kshyatt before, this PR consists of two changes:
The first (set) is:
TensorMapare now no longer doing the checks, bypassing a small amount of overhead for looking up some of the cached data, as well as a small amount of overhead for the warnings withmaxlog = 1.The second is:
sectorequalandsectorhashcan be specialized forTrivialsymmetry type, as brought up in Unnecessary Set allocation in sectorequal/issetequal for Trivial sectors #475.Note that this is only a partial fix for #475, and to thoroughly fix some of this we would probably rather try to avoid caching the symmetry structure, which requires slightly more plumbing which I would like to hold off on for now, if this turns out to not be required it's a bunch of specialized code to maintain and check.