-
Notifications
You must be signed in to change notification settings - Fork 34
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
entanglement_entropy
can be a footgun (it has different meaning for Kets and Operators) -- consider deprecating in favor of separate functions
#168
Comments
Thanks for bringing this up! For better or worse, this is expected (and documented (maybe poorly)) behavior.
This seems to be a reasonable description of "operator space entanglement entropy": https://arxiv.org/pdf/2201.05099 Historically the developers of this package have decided on this set of semantics. There are good reasons to do so, and I am reluctant to consider a breaking release that changes the semantics. Either way, this can be more vividly documented (or potentially deprecated in favor of separate I will change the title of this issue and keep it open to keep track of this. |
entanglement_entropy
can be a footgun (it has different meaning for Kets and Operators) -- consider deprecating in favor of separate functions
@Krastanov Thank you for chipping in so quickly! In my humble opinion this is beyond semantics, a state either as a ket |
@david-pl , @amilsted , how do you feel about a potential deprecation and split here? Something like:
We can keep these deprecation warnings for a few years before we act on them. Personally, I do feel the current state, while perfectly consistent and rigorous, would lead to similar confusing experience in the future. |
I'm open to this change, especially if we deprecate the current method. Admittedly, I didn't use this functionality too much myself. However, I see how this can be confusing and that can this will likely trip up any first time user. There's quite a few parts in QO.jl that could do with better usability and I think this is one of them. @acroscarrillo would you be open to put this split in a PR? |
@david-pl I'm more than happy to help but I'm fairly new to collaborative coding. Could you guide me a little bit? I'm okay with the code writing part, it is a simple change |
Thanks, Alejandro! Here is a typical workflow (but not necessarily the only possible workflow). You might already know much of this, but I am typing it out for completeness:
A few notes:
|
@Krastanov thanks for your guidance! I think I've done all the steps and got stuck at the last, I get:
Im doing it using the official VS code "GitHub Pull Requests" extension: |
I believe the issues might be a step before creating the PR. Looking at this screen shot, it is suggesting that you create a pull request for the merging of Double check that your Looking at your github profile I do not see a fork of QuantumOpticsBase.jl (unless you have made a private fork). You need your own fork to which you can write your branches for review before merging into the official repository. I suspect you have made a local clone of the official repository instead of making a clone of your own fork. For instances, here is what I see if I ask my local repository on my computer to tell me what remote repositories it knows of:
|
Edit (by @Krastanov): This is expected behavior. See first comment below for possible ways forward.
Original post below:
There seems to be a bug in the entanglement entropy calculation, it seems like it might be a factor of 2 off?
Here's the code to reproduce the error:
I create the following state and check is normalised:
Now I calculate the entanglement between the A={1,2} and B={3} as follows:
However, this is wrong. I have done the calculation analytically (two methods, SVD and partial tracing) and it should be$-\frac{2}{3}\log(\frac{2}{3})-\frac{1}{3}\log(\frac{1}{3}) \approx 0.6365471166931516$ , which could look like $1.2730283365896256 / 2$ but not exactly?
It's interesting cause your ptrace agrees with my calculations since
and by inspection this spectrum does indeed agree with my calculations. I am a bit puzzled cause in the source code you guys have
so it should be doing the same provided the spectrum is the same, which it should be since it is being passed the same density matrix.
To check I am not crazy:
I guess it could be that I am miss using the
partition
argument, I didn't find the documentation very clear.The text was updated successfully, but these errors were encountered: