-
Notifications
You must be signed in to change notification settings - Fork 990
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
Defensively copy density matrices in simulator #3109
Conversation
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 think "copy on initialization" and "copy from intermediate states" are sufficient - "copy from final state" shouldn't be necessary, since the simulator shouldn't reuse the state beyond that point.
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.
Looks good. Just a few nits and suggestions for more robust tests.
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.
Thanks @cduck I didn't know about np.shares_memory, this is really useful!
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 after fixing the incorrect assertion.
@@ -1134,6 +1136,8 @@ def test_density_matrix_copy(): | |||
traces.append(np.trace(step.density_matrix(copy=False))) | |||
assert any(not np.isclose(np.trace(x), 1.0) for x in matrices) | |||
assert all(np.isclose(x, 1.0) for x in traces) | |||
assert any(not np.shares_memory(x, y) for x, y in |
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.
You mean all(not
or not any(
?
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.
fixed
Automerge cancelled: A required status check is not present. Missing statuses: ['cla/google'] |
Automerge cancelled: A required status check is not present. Missing statuses: ['cla/google'] |
@clabot this should be fine |
During the (ongoing) review of #3630 95-martin-orion@ found a bug in my draft code. He said that I should copy state like in #3109. I changed my code and added a test in that PR. However, I had inspired myself from the Clifford code which could have the same issue (I think), and so I am fixing the code in the present PR.
This makes it so that by default when you access the density matrix during simulation, it makes a copy of the matrix. This can be overridden if one wants to get the speed benefits of not copying.