Skip to content

Refactor initializeMPS#180

Merged
pbrehmer merged 5 commits intomasterfrom
pb-initialize-mps
Apr 21, 2025
Merged

Refactor initializeMPS#180
pbrehmer merged 5 commits intomasterfrom
pb-initialize-mps

Conversation

@pbrehmer
Copy link
Collaborator

Here I rename the function to initialize_mps to make it consistent with our naming conventions and also add an optional function and type arguments such that it has the function signature

initialize_mps(f=randn, T=scalartype(O), O::Union{InfiniteTransferMatrix,MultilineTransferMatrix}, arg)

@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 41.66667% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/operators/transfermatrix.jl 41.66% 7 Missing ⚠️
Files with missing lines Coverage Δ
src/PEPSKit.jl 100.00% <ø> (ø)
src/operators/transfermatrix.jl 66.66% <41.66%> (+1.80%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pbrehmer pbrehmer requested a review from lkdvos April 18, 2025 12:24
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 like this change, however I'm not entirely sold on the syntax (including the one in MPSKit, I just haven't gotten around to tackling that).

The InfiniteMPS(f, T, Pspaces, Vspaces) style was very heavily centered around the fact that you also have TensorMap(f, T, spaces).
TensorKit decided to stray away from that a bit as that is now deprecated, and rather we are using rand(T, spaces), zeros(T, spaces) etc, since that actually resembles a bit more closely what you would expect from Base Julia.

In that sense, even though InfiniteMPS still pretends to take an arbitrary function f, actually it only takes in rand, randn, zeros or something that we can map to functions that we know.

With this in mind, the idea I had been toying with was to actually define custom structs for MPSSpace and PEPSSpace etc that hold the Pspaces and Vspaces, and then simply have rand(T, mpsspace) etc as the interface. This is all very much not worked out and exists solely in my head, but I thought it might be a good place to share this first

@pbrehmer
Copy link
Collaborator Author

TensorKit decided to stray away from that a bit as that is now deprecated, and rather we are using rand(T, spaces), zeros(T, spaces) etc, since that actually resembles a bit more closely what you would expect from Base Julia.

I do like that change in TensorKit and it would make sense to eventually use that convention in MPSKit and PEPSKit as well. However, I'm not sure what the best intermediate solution is here. Given that all the underlying (MPS/PEPS) constructors still use the old type(f, T, spaces...) pattern, It would be quite a big change to make MPSKit and PEPSKit consistent with that new pattern. From a pragmatic point of view, I just wanted something to control the MPS initialization since that can help convergence in some cases.

With this in mind, the idea I had been toying with was to actually define custom structs for MPSSpace and PEPSSpace etc that hold the Pspaces and Vspaces, and then simply have rand(T, mpsspace) etc as the interface.

Sounds like a good idea! While that would definitely be useful in this case I could also see this structure being useful elsewhere. There are many cases where we want to access specific parts of the PEPS space (also from the user side) and this could probably streamline that.

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'm definitely fine with adding this as-is, since this won't hurt me too much in the long run. My main hesitation is that this is definitely something that will make it into scripts, which means that if we then decide to change this we will break everyone's scripts.

This obviously shouldn't stop us from progressing, it's just something to keep in mind when we decide.
TLDR: I'm okay with merging this.

@pbrehmer pbrehmer merged commit 3c7ed46 into master Apr 21, 2025
37 of 39 checks passed
@pbrehmer pbrehmer deleted the pb-initialize-mps branch April 21, 2025 09:44
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.

2 participants