Use TensorKitTensors for pre-defined tensors

[Yue-Zhengyuan]

This PR removes MPSKitModels dependency.

This PR switches the source of pre-defined tensors from MPSKitModels to TensorKitTensors.

Other issues that are also addressed:

• Hamiltonians imported from MPSKitModels get new docstrings that properly describe them as LocalOperator on square lattice for PEPSKit purposes (instead of "MPO ... on an infinite chain with unit lattice spacing" in the original docstrings in MPSKitModels).
• 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/models.jl	75.00% <64.86%> (-5.49%)	⬇️

... and 3 files with indirect coverage changes

🚀 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?

[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?

[VictorVanthilt]

@Yue-Zhengyuan I'm definitely in favor of moving some of our tensors to TensorKitTensors. There are some tensors in our suite that are not really useful to TensorKitTensors (think of the ising tensors that include impurities introduced by @JaridPiceu) so we would end up with a weird mix of internal and re-exported tensors :/ .

[leburgel]

There are some tensors in our suite that are not really useful to TensorKitTensors (think of the ising tensors that include impurities introduced by @JaridPiceu) so we would end up with a weird mix of internal and re-exported tensors :/ .

Since quite a few of them already have a rather "external" origin, I'm sure this would be an acceptable solution :).

[leburgel]

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

I'm personally fine with this, and have by now been convinced that there's no intrinsic benefit to reusing the MPSKitModels model methods. In particular, there's no solution for the method ambiguities that result from importing both PEPSKit and MPSKitModels either way.

The only real issue, which is what I think @lkdvos was originally also concerned with, is that this is a breaking change without that much external benefit. Regardless of whether it's the conceptually correct thing to do, this is a bit annoying. Therefore, it is still a natural question to wonder if it's worth it, even if it's the right thing to do.

I'm personally fine with merging and breaking, but would ask @lkdvos to chime in one last time on this.

[lkdvos]

I really don't see enough benefit to removing this and causing a breaking change, since this can easily be avoided while achieving everything you brought up.
My suggestion is:

• Use TensorKitTensors where applicable, as you have already done here
• Keep MPSKitModels for now, but import MPSKitModels: transverse_field_ising, ... as well as export transverse_field_ising, ...

This latter makes sure that you can write either PEPSKit.transverse_field_ising or MPSKitModels.transverse_field_ising while meaning the same thing, so you don't have to explicitly import MPSKitModels to use PEPSKit, which means that you only end up with naming conflicts if you explicitly using for both, which would not be solved by removing the dependency here anyways.

This also keeps things non-breaking, and allows us to keep some of the lattice stuff alive. You can still attach docstrings to whatever function you like, and the amount of code change work to do this is minimal

[lkdvos]

I'm not super happy with these abbreviations, they seem to me kind of inconsistent (why Hub and not HO, why small tJ?) and I think it would be nicer to just write them out in full, so here just have:

Suggested change

	import TensorKitTensors.SpinOperators as SO
	import TensorKitTensors.FermionOperators as FO
	import TensorKitTensors.HubbardOperators as Hub
	import TensorKitTensors.TJOperators as tJ
	import TensorKitTensors.BosonOperators as BO
	using TensorKitTensors

[Yue-Zhengyuan]

Writing things out in full is a bit painful. I can use HO and TJ (reason for tJ is because it is literally called the $t$-$J$ model), though.

[Yue-Zhengyuan]

Then I'll restore MPSKitModels dependency, only rewrite the Hamiltonians (give me a few days to go back to this).

[lkdvos]

Thanks for taking the comments into account, looks good to me!

[Yue-Zhengyuan]

Let's wait a bit longer for TensorKitTensors v0.3.0.

[lkdvos]

I don't think there are any external changes there right, so that should just be compatible with both?

[Yue-Zhengyuan]

I don't think there are any external changes there right

Indeed, then merging now is also fine. There is a weird timeevol test failure that just seems to be stuck somewhere instead of actually errored.

[Yue-Zhengyuan]

Though I do want to directly start with v0.3.0 in case there are some deeply-hidden wrong tensor elements like QuantumKitHub/TensorKitTensors.jl#47 (this is already patched in v0.2.5, though), which should now be all gone with the rewrite.

[lkdvos]

I don't have any strong preference, the benefit of just having compatibility with both is that it doesn't block if there are other packages that haven't released their compat upgrade yet, the drawback is that people might still be using old versions that contain bugs. There's arguments for both 🤷