Skip to content

[Perf] reuse renormalize_west_edge for all directions#237

Merged
lkdvos merged 7 commits intoQuantumKitHub:masterfrom
ogauthe:renormalize_north
Jul 29, 2025
Merged

[Perf] reuse renormalize_west_edge for all directions#237
lkdvos merged 7 commits intoQuantumKitHub:masterfrom
ogauthe:renormalize_north

Conversation

@ogauthe
Copy link
Contributor

@ogauthe ogauthe commented Jul 26, 2025

This PR rewrites renormalize_north_edge, renormalize_east_edge and renormalize_south_edge to have them reuse renormalize_west_edge. renormalize_west_edge was rewritten in #229 to optimize the permutations. This PR fixes #220, part of #205.

  • the formatter made changes in the tensor diagrams. I can undo them if you prefer minimizing the diff
  • I added a test for a 4x4 unit cell with each bond different

Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

I don't mind too much about the changes in the diagrams so feel free to leave them in if you like.

Is there any chance you could run a small benchmark comparing the performance of the different directions now, so we can easily conclude that the different directions have more or less the same performance?

@codecov
Copy link

codecov bot commented Jul 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.

Files with missing lines Coverage Δ
src/algorithms/contractions/ctmrg_contractions.jl 55.24% <100.00%> (-0.39%) ⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ogauthe
Copy link
Contributor Author

ogauthe commented Jul 27, 2025

Is there any chance you could run a small benchmark comparing the performance of the different directions now, so we can easily conclude that the different directions have more or less the same performance?

for SU(2) D=11, median time using https://github.com/ogauthe/BenchmarkPEPS/blob/main/run/benchmark_ctmrg.jl
renormalize_north_edge: 190.0 ms
renormalize_east_edge: 192.2ms
renormalize_south_edge: 191.6 ms
renormalize_west_edge: 189.4 ms
it was 295 ms before this PR

SU(2) D=16:
renormalize_north_edge: 2.87 s
renormalize_east_edge: 2.91 s
renormalize_south_edge: 2.91 s
renormalize_west_edge: 3.08 s
it was 5.2 s before this PR

So for SU(2) at D=11 and D=16, the cost of the additional sandwich permute is smaller than the fluctuations in benchmark time.

EDIT: same story for U(1) at D=11
the 4 direction take 37 ± 1 ms

@lkdvos
Copy link
Member

lkdvos commented Jul 28, 2025

with the expected compile-time benefits, that's definitely sufficient for me.

I'll try and investigate why the docs build is failing, I'm not sure if this can be related to this PR

@lkdvos
Copy link
Member

lkdvos commented Jul 28, 2025

It seems like the documentation build is really just a result of a Documenter.jl update. I'm rerunning the MPSKitModels docs to see if that fixes it, but in the meantime, this should not hold back this PR.

@ogauthe
Copy link
Contributor Author

ogauthe commented Jul 28, 2025

I removed the partition function methods. The 2D Ising partition function example works, so I guess legs ordering is fine.

@lkdvos lkdvos requested review from leburgel and pbrehmer July 28, 2025 22:55
Copy link
Member

@lkdvos lkdvos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this looks like a good change: it improves overall performance while reducing the amount of code.
@leburgel or @pbrehmer , if there are no further opinions on this I will go ahead and merge this end of tomorrow, if the tests pass.

Copy link
Member

@leburgel leburgel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@lkdvos lkdvos merged commit 8d7e8a4 into QuantumKitHub:master Jul 29, 2025
42 of 45 checks passed
@ogauthe ogauthe deleted the renormalize_north branch July 29, 2025 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Projectors performance improvements

3 participants