Skip to content

Conversation

@Yue-Zhengyuan
Copy link
Member

@Yue-Zhengyuan Yue-Zhengyuan commented Oct 11, 2025

This PR generalizes correlator to calculate correlation functions ⟨O(i, j)⟩ between two sites (i, j) in the same row/column in a 1-layer InfinitePEPO ρ (mixed state). Only tr(ρO) is implemented. ⟨ρ|O|ρ⟩ is not intended to be treated here.

To do:

  • Figure out the twists required for fermions.

@codecov
Copy link

codecov bot commented Oct 11, 2025

Codecov Report

❌ Patch coverage is 90.21739% with 18 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/algorithms/contractions/transfer.jl 68.75% 10 Missing ⚠️
src/algorithms/correlators.jl 94.64% 3 Missing ⚠️
src/algorithms/transfermatrix.jl 80.00% 3 Missing ⚠️
src/algorithms/toolbox.jl 93.75% 1 Missing ⚠️
src/operators/infinitepepo.jl 66.66% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/PEPSKit.jl 100.00% <ø> (ø)
.../algorithms/contractions/correlator/pepo_1layer.jl 100.00% <100.00%> (ø)
src/algorithms/contractions/correlator/peps.jl 100.00% <100.00%> (ø)
src/algorithms/contractions/vumps_contractions.jl 84.53% <100.00%> (-0.32%) ⬇️
src/algorithms/ctmrg/gaugefix.jl 97.19% <100.00%> (ø)
src/algorithms/toolbox.jl 97.69% <93.75%> (-0.70%) ⬇️
src/operators/infinitepepo.jl 77.00% <66.66%> (+5.57%) ⬆️
src/algorithms/correlators.jl 96.87% <94.64%> (-3.13%) ⬇️
src/algorithms/transfermatrix.jl 80.00% <80.00%> (ø)
src/algorithms/contractions/transfer.jl 68.75% <68.75%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Yue-Zhengyuan Yue-Zhengyuan marked this pull request as ready for review October 11, 2025 03: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.

Overall looks good to me!

I do think I slightly disgree with putting the logic for the correlators in the contractions folder. If possible, I think I would put only the contractions there, while keeping the actual correlator logic in a separate file.

@Yue-Zhengyuan
Copy link
Member Author

I can move the redefinition MPSKit.correlator to src/algorithms/toolbox.jl without changing other functions. But I don't quite want to scatter things around. BTW there are also many things in src/algorithms/toolbox.jl that appear worth putting into src/algorithms/contractions.

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.

I think my original motivation for the contractions folder was to have a place to put an implementation for some of the contractions that clutter up the main logic of the algorithms. So in some sense, I would have put only the @tensor calls in there, and the generated functions.
I do think the algorithms/correlators.jl file made sense, so I would rather keep that logic there if you don't mind.

I additionally had a look in a bit more detail about the transfer logic, and I think it would make more sense to follow the previous/other strategies a bit closer, rather than using _transfer_left directly.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2025

Your PR no longer requires formatting changes. Thank you for your contribution!

Copy link
Member Author

@Yue-Zhengyuan Yue-Zhengyuan left a comment

Choose a reason for hiding this comment

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

I have moved correlator_horizontal, correlator_vertical and the extension of MPSKit.correlator back to src/algorithms/correlators.jl.

To really using the V * T syntax, I need some help in figuring out the fermion twists to be added to the planar transfer_left.

@leburgel
Copy link
Member

leburgel commented Oct 28, 2025

Okay, to move forward with this. While it does kind of stall progress here, I think it's not too much of a detour to properly address the transfer function confusion between MPS and CTMRG environments here. I would suggest to

  • Introduce dedicated methods for transfer functions using CTMRG edge tensors (i.e. without adjoints)
    • My first idea would be to call these edge_transfer_left and edge_transfer_right, but I would be happy with any reasonable name
  • Replace all instances which call PEPSKit._dag before MPSKit.transfer_left/right with direct calls to the edge transfer methods
  • Double check to only call MPSKit.transfer_left/right on objects which adhere to the MPSKit convention for parity flips, to avoid any possible future confusion
  • [Optional, at least to me for merging this PR] Wrap the application of edge_transfer_left/right into multiplication syntax for a EdgeTransferMatrix <:MPSKit.AbstractTransferMatrix (name up for debate) which would be the equivalent of MPSKit.SingleTransferMatrix
    • I don't think this is strictly necessary for this specific PR, but it's probably not much extra work so if it turns out to be easy we might as well add it here.
    • Unfortunately, here we can't really use the generic constructor TransferMatrix(...), since the objects we would call it on are type-indistinguishable from the ones used in MPS transfer matrices. So this part would require messing around a bit with syntax and signatures

I can give this a go tomorrow morning. Having originally introduced the _dag shortcut, I'm happy to help dealing with the resulting fallout :)

As a side note, having to think about all of this again made me realize that probably our correlation length implementation is most likely just wrong for fermionic systems. We currently compute a transfer matrix spectrum using (in the end) MPSKit.transfer_left, but this is actually not compatible with the twist convention we use for the south CTMRG edges. So also there, we need to either pre-twist the south edges before passing them to MPSKit.transfer_spectrum, or just compute the spectrum of a proper edge transfer function in the first place.

@Yue-Zhengyuan
Copy link
Member Author

I suggest the slightly more verbose names ctmedge_transfer_left/right and CTMEdgeTransferMatrix (or omitting "edge").

@lkdvos
Copy link
Member

lkdvos commented Oct 28, 2025

I don't think this is linked to CTM though, I would argue that actually there might even be a case to be made for just using PEPSKit.TransferMatrix and distinguishing that from MPSKit.TransferMatrix... But for clarity, probably EdgeTransferMatrix is fine?

@leburgel
Copy link
Member

leburgel commented Oct 29, 2025

I had a go at the edge transfer matrices like I said. I think I changed all the relevant bits, and also updated the correlation length implementation in the process so that should now work for symmetries with non-trivial twists. Some things I'm not entirely sure about:

  • I removed a bunch of twists from the correlator implementation, since none of these now use @plansor for the contractions anymore. The only explicit twists left are in start_correlator and end_correlator_numerator, is this correct @Yue-Zhengyuan?
  • The names are all up for debate still. In particular, I introduced edge_transfermatrix in anticipation of an MPSKit.transfermatrix that will be added and would clash with the argument types, but I don't like this name much
  • There's also the question of how much sense it makes to choose EdgeTransferMatrix <: MPSKit.AbstractTransferMatrix; it's hard to say how much gain there is over added confusion if we don't reuse some of the infrastructure.

@Yue-Zhengyuan
Copy link
Member Author

There are problems when the physical space is a dual space. I'll try to solve them later.

@Yue-Zhengyuan
Copy link
Member Author

@lkdvos I'm now sure that either str or trmul is wrong when the physical spaces include dual spaces

using Random
using TensorKit
using PEPSKit
Random.seed!(0)
V1 = Vect[FermionParity](0 => 2, 1 => 2)
V = V1'  V1'  V1
# create a positive operator
ρ = randn(V  V)
ρ = ρ * ρ'
A = id(V)
PEPSKit.trmul(A, ρ) / PEPSKit.str(ρ) # not equal to 1

@lkdvos
Copy link
Member

lkdvos commented Oct 30, 2025

Yeah, these functions don't actually do anything particularly smart, I think they really just build the @tensor expressions for these, so no fancy twisting

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.

I added some commentary on things that I think should be reviewed again, other than those this seems good to go for me.

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.

Overall looks good to me, I'm fine with the names, I dont think there are that many better options and I also don't think it matters too much. I have some final small comments, but otherwise should be good to merge for me

@leburgel leburgel merged commit fa943d8 into QuantumKitHub:master Nov 6, 2025
51 checks passed
@Yue-Zhengyuan Yue-Zhengyuan deleted the correlator-pepo branch November 7, 2025 01:14
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.

3 participants