Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change default site value for FiniteMPS in entanglement_spectrum #245

Merged
merged 2 commits into from
Feb 5, 2025

Conversation

VictorVanthilt
Copy link
Contributor

As pointed out by @borisdevos, calling entanglement_spectrum(::FiniteMPS) does not work as the default value for site is 0, which would call the C tensor left of site 0, which does exist, it is just a trivial 1, but throws a BoundsError due to the checkbounds in entanglement_spectrum.

Currently I just split off the entanglement_spectrum method that dispatched on FiniteMPS to have a default site value of 1. This works but now the docstring is not consistent as it shows a default value of 0 for both methods.

In my opinion having a default value for the finite case really does not make sense and the user should just choose the site according to their needs.

Any opinions?

@VictorVanthilt VictorVanthilt requested a review from lkdvos February 5, 2025 10:08
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/algorithms/toolbox.jl 75.00% 1 Missing ⚠️
Files with missing lines Coverage Δ
src/algorithms/toolbox.jl 94.78% <75.00%> (-1.37%) ⬇️

... and 2 files with indirect coverage changes

@lkdvos
Copy link
Member

lkdvos commented Feb 5, 2025

This is definitely ok for me, and removing the default value for the FiniteMPS also seems reasonable.
It might also be a good idea to check if we are using site as a kwarg or a regular argument in other parts of the code, at least for me I know that this often confuses me

@VictorVanthilt
Copy link
Contributor Author

Ok, then I will change it to no default argument for FiniteMPS and I will try to reflect it in the docstring.

@VictorVanthilt
Copy link
Contributor Author

@lkdvos What do you think of this implementation? (The one I just pushed.)

@VictorVanthilt VictorVanthilt merged commit 4d975c0 into master Feb 5, 2025
27 of 28 checks passed
@VictorVanthilt VictorVanthilt deleted the vv/entanglement_spectrum branch February 5, 2025 13:27
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.

2 participants