Fix Multiline (MultilineMPS/MultilineMPO) correctness and API inconsistencies - #508
Open
borisdevos wants to merge 19 commits into
Open
Fix Multiline (MultilineMPS/MultilineMPO) correctness and API inconsistencies#508borisdevos wants to merge 19 commits into
Multiline (MultilineMPS/MultilineMPO) correctness and API inconsistencies#508borisdevos wants to merge 19 commits into
Conversation
…referring to the 2d shape
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I found a cluster of bugs that all traced back to the same root cause, namely
Multiline{T}was inconsistent about what it is. Sometimes it was treated as a 2D array indexed by(row, col), and sometimes as a 1D sequence ofT-typed lines, this while different parts of the code picked different conventions without checking consistency. The real bugs were:MultilineMPO * MultilineMPShad never worked (straight up method error), and fixing that trivially still left both a shape bug (lengthcountingrows*colswhilemap/collectonly ever producedrowselements) and a silently wrong row-shift bug (fidelity 0where it should be1).expectation_value(::MultilineMPS, ::MultilineMPO, ...)had a fallback method that silently returned something meaningless (prodinstead ofsum, no row shift, discardedenvswhich was noted through a todo) for anyMultilineMPOline type it wasn't explicitly guarded against. Related to this, there was half-support for Hamiltonian or finite MPO multilines even though they're meaningless. Also the environments weren't being correctly converted, so they were being recalculated every row.isfinite,changebonds(::MultilineMPO, ::SvdCut), andaxes(m, 2)all threw outright.A summary of the changes I made:
"Semantics":
Multiline{T}now consistently treatslength/eltype/iterate/m[i]as referring to the lines (matching the periodic rows), whilesize/axes/eachindexrefer to the(nrows, ncols)lattice shape. These two views are intentionally different (length(m) != prod(size(m))). I also documented this in docstrings.expectation_valuefallback. Changed the infinite MPS-MPO expectation value to correctly account for environments.isfinite,changebonds(::MultilineMPO/::MultilineMPS, ::SvdCut), andaxes(m, i).spacetype/sectortype/storagetypefor multiline. I needed this for the custom show (see below).Construction restrictions:
MultilineMPOonly acceptsInfiniteMPOlines now, enforced at constructor level. This is deliberate, sinceMultilineis also used via converts in environments, quasiparticles etc.AbstractMatrixconstructor that silently built finite-lineMultilineMPOs, and the dead code inorthoview.jlthat existed only to support finite MPS multilines.Display:
MultilineMPS/MultilineMPOcustom show: added asummarywhich looks at the 2d shape, and ashowthat renders each row using that row's own existingshowmethod. Here below an example of how it looks now:Here the multiline MPS:
Details
And here a multiline MPO:
Details
Tests:
test/operators/multilinempo.jl: exact-fidelity tests for the row-shift convention in bothMultilineMPO * MultilineMPSandMultilineMPO * MultilineMPO, using a permutation MPO whose fixed point is known exactly.test/algorithms/statmech.jl: multi-rowexpectation_valueregression test. Also a test that the Hamiltonian loophole above still correctly throws.test/states/multilinemps.jl: added instance-levelisfinite(ψ)(only the type-level form was covered before).Documentation:
docs/src/man/states.md: added the row-shift convention, what's currently supported (infinite lines only), and subtleties.docs/src/man/operators.md: newMultilineMPOsection covering the same convention, and emphasising the MPO*MPO non-composability caveat.Multiline,MultilineMPO,MultilineMPS, andexpectation_value.Some things I didn't address because I'm not sure what to do with them:
MultilineMPS, simply becauseInfiniteMPSitself doesn't implementzerovector/scale.norm(ψ) == sqrt(nrows)for aMultilineMPSof individually-normalized rows is unchanged. This goes throughdot/innersum over rows. I don't know if this is what you want.Checklist
julia --project=test test/runtests.jl, or the relevant subset)docs/src/)[Unreleased]indocs/src/changelog.md, if this PR is user-facing (new feature, behavior change, bug fix, deprecation, or removal)