More informative errors for finite mps

[borisdevos]

Small silly change, but while I was being lazy with variable names and confused my finite MPS with infinite MPS, I realised I could call certain functions and get it to error at weird places. This change fixes that. It seemed good to do since these functions are exported, so the generic method was always available.

Unrelated to this, I was also playing around with Cartesian spaces and transfer_spectrum would use the sector Trivial() to make a ComplexSpace, so now it checks the space type instead of sector type.

Final thing to add: the eigsolve that happens in transfer_spectrum defaults to Arnoldi even though the state is fully real. I didn't change this, because I simply don't know how much support we want for real MPSs.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
src/algorithms/toolbox.jl	50.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
src/algorithms/toolbox.jl	95.15% <50.00%> (ø)

... and 4 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]

I'm not entirely sure how I feel about the Vector restrictions.
At the very least it would have to be AbstractVector{<:Number}, but I am not sure this really solves your issue: instead of whatever error you had previously you now just get a MethodError. Is there any other input checking we can do instead, to actually give a reasonable message?

For the Arnoldi method, this has nothing to do with the state being real, and rather that our transfer matrix isn't hermitian (or symmetric), so Lanczos would actually give wrong results there.

[borisdevos]

The error I had previously was vague enough with an unnecessarily long stack-trace that I thought it was justifiable to raise the method error from the beginning. I'm not sure what other kind of message can be raised. Transfer matrices do exist for finite MPS but their spectra are not so simply solved. This isn't really a hill to die on from my side, so I don't mind whatever.

[lkdvos]

But I'm confused about where you are putting the restrictions, would that not mean you should restrict the functions that call these methods to not accept FiniteMPS?

[borisdevos]

I'm confused as well now, because isn't that what the method error does?

[lkdvos]

Woohoow, spreading confusion 🎉

What I mean is, you are now restricting correlation_length to only accept vectors, while I would expect you to define correlation_length(::AbstractMPS, ...) or something similar to throw an error if it is a FiniteMPS, which has a message stating you can't do that :)

[borisdevos]

I could do that (and I'm not against it), but I guess I don't understand how that's more informative than having the correlation_length(::InfiniteMPS) exist like it does now, but restrict the other method which allowed FiniteMPS to be its argument. Similarly for marek_gap.

[lkdvos]

It doesn't matter too much, I'm also happy with the AbstractVector kind of approach, but in general the idea is to restrict types the least amount possible. For example, in principle we could decide at some point to diagonalize the full spectrum of the transfer matrix, which would then be a SectorDict, and perhaps the methods might still just work. I also think an explicit error that says you can't call correlation_length on a FiniteMPS is more informative than a MethodError, since that might also mean that it isn't implemented, or you accidentally have the argument order wrong, etc. None of this is super important though, so feel free to merge this if you think it isn't worth the hassle, I'm just giving my general thoughts :)

[borisdevos]

Thanks for sharing your thoughts! For now, I'll merge as is, and deal with potential repercussions in the future 😉