Remove MPSKitModels dependency

[Yue-Zhengyuan]

This PR removes MPSKitModels dependency. The main purpose is to use pre-defined operators from TensorKitTensors.

• Now all Hamiltonians are decoupled from the ones from MPSKitModels.
• The general machinery on AbstractLattice is lost. But I think this will eventually be replaced by TiledArrays?

Some issues that are addressed:

• J1-J2 model shouldn't have spin U(1) symmetry with sublattice = true (S_xx, S_yy with U1Irrep symmetry MPSKitModels.jl#57). With TensorKitTensors, an error will be correctly raised.
• pwave_superconductor is actually a p-ip (instead of p+ip) wave Hamiltonian. I wonder if we want to change the convention and update the benchmark energy in the test.

[codecov]

Codecov Report

❌ Patch coverage is 64.86486% with 13 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/operators/models.jl	64.86%	13 Missing ⚠️

Files with missing lines	Coverage Δ
src/operators/lattices/squarelattice.jl	95.00% <ø> (ø)
src/operators/models.jl	75.00% <64.86%> (-5.49%)	⬇️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[lkdvos]

Definitely in favor of using TensorKitTensors for the operators, but I'm not entirely sure if we gain a lot by decoupling the actual models. The main thing I worry about is that there are now two functions for each model, and you have to remember that PEPSKit.hubbard_model !== MPSKitModels.hubbard_model.

We should however probably discuss this in a bit more detail together, since I agree that the MPSKitModels package is a bit awkward. I would be happy to include most of that functionality in MPSKit, for example, since I feel like it does not actually warrant a separate package, but in that case probably we should move out the lattice stuff in a different package (ie. TiledArrays/LatticeArrays or something similar).

[Yue-Zhengyuan]

you have to remember that PEPSKit.hubbard_model !== MPSKitModels.hubbard_model.

This is easy 😂 Actually for newcomers I don't think they expect MPSKitModels to provide 2D Hamiltonians?

[lkdvos]

Well, the point is that if you accidentally do something like using MPSKitModels, PEPSKit, since both these functions are exported and have the same name, we now get a clash, your REPL will probably spit out some warnings, and you now have to explicitly qualify from which you want to import the function. I'm not saying this is horrible, but this is definitely a breaking change, and I'm not entirely sure what we are gaining by not including MPSKitModels, since we depend on MPSKit anyways

[Yue-Zhengyuan]

you now have to explicitly qualify from which you want to import the function

Before TensorKitTensors exists, I needed to import MPSKitModels to access pre-defined operators in order to define custom Hamiltonians. This was already causing a name clash on LocalOperator, and I have to explicitly write using PEPSKit: LocalOperator. But now we can import TensorKitTensors instead, so one big reason to import both MPSKitModels and PEPSKit is gone.

this is definitely a breaking change, and I'm not entirely sure what we are gaining by not including MPSKitModels, since we depend on MPSKit anyways

Dependence on MPSKit is quite reasonable, since it should be common knowledge that PEPS can be contracted with boundary MPS methods. But getting PEPSKit Hamiltonians from MPSKitModels has always weirded me out.

After rewriting the Hamiltonians with TensorKitTensors, what remains that still depends on MPSKitModels is just the AbstractLattice stuff. Since we are currently supporting square lattice only, I think it's not worth it to keep depending on another package that is not designed to work with PEPS. Although this will be a breaking change, the function signatures remain untouched, so the difference is not that dramatic.

[leburgel]

Using TensorKitTensors over MPSKitModels for the operators is a strict improvement, no doubt about that.

Regarding the models, it is conceptutally a bit strange to rely on MPSKitModels just to overload the model methods. However, I think it's almost equally strange to introduce new methods with exactly the same name and signature, and even literally copy over the docstrings from MPSKitModels. Getting PEPSKit Hamiltonians from MPSKitModels is indeed weird, but given that the MPSKitModels ones already exist there's also no real reason not to reuse them.

At the end of the day, the main issue is that using operators from MPSKitModels is not a good idea. This is resolved here. The fact that the model names originate from MPSKitModels is not really visible from the outside, since we re-export all of the model methods from PEPSKit. The naming clashes that arise from using PEPSKit, MPSKitModels aren't solved by relying on MPSKitModels or not, either way this leads to trouble. As long as this PR takes away the need to do using MPSKitModels when working with PEPSKit, that solves the problem as far as I'm concerned.

So I would be in favor of keeping the MPSKitModels dependency. Even if it's just not to copy over the model docstrings, this is already worth it to me. Then we just bury the actual dependency so we don't give anyone a reason to ever import MPSKitModels themselves. This includes not mentioning the model methods come from MPSKitModels in the documentation, which is already done in this PR.

At that point, are there still any real downsides to having MPSKitModels as a dependency?

[leburgel]

For example, here we could keep the updated text to not refer to MPSKitModels, but just change the (@ref) to (@extref MPSKitModels.bose_hubbard). This is not visible to readers, but allows to reuse at least the docstring.

[Yue-Zhengyuan]

it's almost equally strange to introduce new methods with exactly the same name and signature, and even literally copy over the docstrings from MPSKitModels

The point is that we don't need to worry about the two packages becoming out of sync. For example, if we want to add next-nearest neighbor options (like t2 for Hubbard/t-J), we are not forced to update MPSKitModels simultaneously to match the docstring. If we eventually have native support for other lattices (triangular, kagome, etc) the discrepancy will only become larger.

So it looks like duplicating things right now, but is more future-proof in my opinion.

[Yue-Zhengyuan]

Moreover, the model docstrings in MPSKitModels say they are "MPO for the hamiltonian". So they clearly do not apply to PEPSKit, and I have made the adjustments.

[Yue-Zhengyuan]

For the environment initialization test with partition function of Ising model, I suggest we duplicate the Z2Irrep version of classical_ising here (which is not very long) and only test this case.

[leburgel]

Moreover, the model docstrings in MPSKitModels say they are "MPO for the hamiltonian". So they clearly do not apply to PEPSKit, and I have made the adjustments.

I didn't notice this, that is unfortunate. This does kind of defeat the point of my earlier comment. If there's nothing that can be reused, forcing reuse is indeed not a good idea.

For the environment initialization test with partition function of Ising model, I suggest we duplicate the Z2Irrep version of classical_ising here (which is not very long) and only test this case.

This I really don't like, to be honest. Even though it's not very long, that would mean defining exactly the same tensor in three different packages within our own organization. If we keep copying it over from MPSKitModels because we don't want to depend on it, clearly something is wrong and it shouldn't have been defined there in the first place. But then at least we should move it to TensorKitTensors, and import it from there.

[Yue-Zhengyuan]

Then for now we depend on MPSKitModels only for the tests.

at least we should move it to TensorKitTensors, and import it from there.

This sounds good. @VictorVanthilt How do you feel about also moving some "widely used" classical 2D partition function tensors from TNRKit to a new submodule (maybe naming it PartitionFunction) in TensorKitTensors?