Skip to content

Consistent environments signature: below, operator, above argument order#268

Merged
lkdvos merged 3 commits intomasterfrom
lb/approximate_bug
Mar 31, 2025
Merged

Consistent environments signature: below, operator, above argument order#268
lkdvos merged 3 commits intomasterfrom
lb/approximate_bug

Conversation

@leburgel
Copy link
Member

Changes the syntax convention for infinite MPS environment-related methods to (below, operator, above=below), in line with the finite MPS convention. Since above is always the optional one from the point of view of boundary MPS algorithms, this made more sense. I didn't change any of the TransferMatrix related code, which still assumes (above, operator, below=above) for its arguments. I suspect changing this would cause a lot more downstream issues.

In the process I noticed that calc_galerkin would have been wrong when used in approximate for finite MPS, and after that noticed that we don't actually use the proper Galerkin error in the finite approximate. I think we could, and probably should? But this can be addressed in a follow-up I think.

Fixes #267.

@codecov
Copy link

codecov bot commented Mar 30, 2025

Codecov Report

Attention: Patch coverage is 95.31250% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/environments/multiline_envs.jl 80.00% 3 Missing ⚠️
Files with missing lines Coverage Δ
src/algorithms/toolbox.jl 95.04% <100.00%> (ø)
src/environments/abstract_envs.jl 91.89% <100.00%> (ø)
src/environments/infinite_envs.jl 100.00% <100.00%> (ø)
src/environments/multiple_envs.jl 79.31% <100.00%> (ø)
src/operators/multipliedoperator.jl 55.00% <100.00%> (ø)
src/environments/multiline_envs.jl 85.36% <80.00%> (ø)

... and 1 file with indirect coverage changes

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

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.

Great changes, thanks!
Indeed, when I previously looked at this I realized there were definitely some issues but didn't feel like untangling that mess, but this looks relatively contained.

Slightly off-topic, but I wanted to share my view anyways:
For the transfer stuff, I'm a bit on the fence whether or not we should also change that (definitely not in this PR). Especially for the actual structs it seems like top middle bottom is not a strange order of the fields.
I think I would for that case prefer to just introduce a function transfermatrix that has similar syntax to our other functions which returns the object, and as usual the fields of that object are considered implementation details so that should work out.

@lkdvos lkdvos enabled auto-merge (squash) March 31, 2025 11:28
@lkdvos lkdvos disabled auto-merge March 31, 2025 11:32
@lkdvos lkdvos changed the title Change environments calling syntax for inifinite MPS Consistent environments signature: below, operator, above argument order Mar 31, 2025
@lkdvos lkdvos enabled auto-merge (squash) March 31, 2025 11:33
@lkdvos lkdvos merged commit d30ef9e into master Mar 31, 2025
28 checks passed
@lkdvos lkdvos deleted the lb/approximate_bug branch March 31, 2025 11:33
@lkdvos lkdvos mentioned this pull request Mar 31, 2025
5 tasks
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.

A bug?

2 participants

Comments