-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Fix phase management in StabilizerState.expectation_value
#7460
Conversation
8ab83ec
to
3f172db
Compare
Pull Request Test Coverage Report for Build 1858344895
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Let me make just one issue.
|
||
Args: | ||
oper (BaseOperator): an operator to evaluate expval. | ||
oper (Pauli): a Pauli operator to evaluate expval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we extend this to SparsePauliOp, we can use from_operator to apply it to the entire BaseOperator
. However, I think this is beyond the scope of the bug fix and should be treated as a separate issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should open a separate PR to extend this to SparsePauliOp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can start such a PR after this PR will be merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle the change here looks straightforwards, but I don't actually know the maths, and there's an unchanged line that feels wrong. If it's actually correct, I'm happy to accept if we can beef up the new test case a little further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for adding tests. I think it's more robust now. Maybe there are some things that could be improved that this PR haven't changed, but I think this is fine.
@jakelishman What do you think?
I just changed the tests to follow the ddt patter. For the rest, I trust @ikkoham with the math. Let's get it merged. |
StabilizerState.expectation_value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry that I took so long to come back to this.
The tests look much more complete now, thanks - I've got way more confidence that we're doing the right thing in a lot more circumstances, even without in-depth knowledge of the algorithm.
I just have one outstanding comment/question in the algorithm. I'll approve now to save time in case the comment's way off base, but if not, it might be good just to put a shade more explanation at the bit I mentioned to help future reviewers (like me) who aren't 100% certain about the formalism.
@jakelishman - here is the algorithm of the expectation value calculation that we got from Sergey and implemented here and in Aer's stabilizer simulator. Suppose one needs to compute <psi|P|psi> where P is a Pauli operator (with no phase) and psi is a stabilizer state with stabilizer generators S_1,...,S_n and destabilizer generators D_1,...,D_n. If P anti-commutes with some stabilizer S_j then <psi|P|psi>=0. Otherwise, P or -P is an element of the stabilizer group and thus P = (-1)^a S_1^{b_1} S_2^{b_2} ... S_n^{b_n} Note that b_j=1 iff P anti-commutes with the destabilizer D_j. |
Thanks, I think I get it all now. I was getting all confused because we were calculating with a counter on the symplectic phase, but at the end we were only either leaving or negating the return value. The idea that it needed to remain real was fine for my intuition, but while I thought that all values of |
* fix a bug in expectation value. Handle Pauli phases. * extend the test to include Pauli expectation vals with phases. * add release notes * add more 1-qubit and 2-qubit tests for exp_val=-1,i,-i * more ddt * Fix typo in error message * Add comment on phase Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> Co-authored-by: Luciano Bello <bel@zurich.ibm.com> Co-authored-by: Jake Lishman <jake@binhbar.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> (cherry picked from commit 6da136c)
…7675) * fix a bug in expectation value. Handle Pauli phases. * extend the test to include Pauli expectation vals with phases. * add release notes * add more 1-qubit and 2-qubit tests for exp_val=-1,i,-i * more ddt * Fix typo in error message * Add comment on phase Co-authored-by: Ikko Hamamura <ikkoham@users.noreply.github.com> Co-authored-by: Luciano Bello <bel@zurich.ibm.com> Co-authored-by: Jake Lishman <jake@binhbar.com> Co-authored-by: Jake Lishman <jake.lishman@ibm.com> (cherry picked from commit 6da136c) Co-authored-by: Shelly Garion <46566946+ShellyGarion@users.noreply.github.com>
Summary
close #7441
Fix a bug in
StabilizerState.expectation_value()
method when thePauli
operator has a non-trivial phase.Details and comments
In addition, verify that the operator in indeed a
Pauli
, and extend the tests accordingly.