Fix process-based non-determinism in SparsePauliOp.to_matrix
#13439
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The simplification step of the summed Pauli terms in `SparsePauliOp.to_matrix` had introduced a non-determinism via hash-map iteration. In practice, the base hash seed was set per initialisation of the extension module, then stayed the same. This is hard to detect from within tests, but caused unexpected numerical behaviour on different invocations of the same script.
Collaborator
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11840492573Details
💛 - Coveralls |
mtreinish
reviewed
Nov 14, 2024
| /// have become zero are dropped. | ||
| pub fn combine(&mut self) { | ||
| let mut hash_table = HashMap::<(u64, u64), Complex64>::with_capacity(self.coeffs.len()); | ||
| let mut hash_table = IndexMap::<(u64, u64), Complex64>::with_capacity(self.coeffs.len()); |
Member
There was a problem hiding this comment.
Do you want to use ahash for the hasher here so the lookup performance is basically the same as HashMap?
Member
Author
There was a problem hiding this comment.
It should have no meaningful effect on the whole algorithm's performance because this bit is immeasurable compared to the cost of constructing the matrix, but the only reason I didn't do it is because I forgot.
My kids are awake now, so if somebody wanted to push up that change in time to get this into 1.3.0, it's fine by me - I won't have time til tomorrow.
kevinhartman
approved these changes
Nov 14, 2024
Contributor
kevinhartman
left a comment
There was a problem hiding this comment.
LGTM. I don't feel strongly that this needs a test given the circumstances.
If anyone disagrees, we can add one in a separate PR.
mergify bot
pushed a commit
that referenced
this pull request
Nov 14, 2024
* Fix process-based non-determinism in `SparsePauliOp.to_matrix` The simplification step of the summed Pauli terms in `SparsePauliOp.to_matrix` had introduced a non-determinism via hash-map iteration. In practice, the base hash seed was set per initialisation of the extension module, then stayed the same. This is hard to detect from within tests, but caused unexpected numerical behaviour on different invocations of the same script. * Use ahash::RandomState --------- Co-authored-by: Kevin Hartman <kevin@hart.mn> (cherry picked from commit 74b32c9)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 14, 2024
… (#13440) * Fix process-based non-determinism in `SparsePauliOp.to_matrix` The simplification step of the summed Pauli terms in `SparsePauliOp.to_matrix` had introduced a non-determinism via hash-map iteration. In practice, the base hash seed was set per initialisation of the extension module, then stayed the same. This is hard to detect from within tests, but caused unexpected numerical behaviour on different invocations of the same script. * Use ahash::RandomState --------- Co-authored-by: Kevin Hartman <kevin@hart.mn> (cherry picked from commit 74b32c9) Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The simplification step of the summed Pauli terms in
SparsePauliOp.to_matrixhad introduced a non-determinism via hash-map iteration. In practice, the base hash seed was set per initialisation of the extension module, then stayed the same. This is hard to detect from within tests, but caused unexpected numerical behaviour on different invocations of the same script.Details and comments
This fixes the bug part of #13413, but doesn't address the feature request. It should be eligible for backport to either 1.3.0 or 1.3.1, depending on whether there's time to make 1.3.0.
Writing a test that would exercise this would be pretty complex, so I haven't done it. I can if we feel strongly about it - we'd likely embed a little script into a test and spawn some
sys.executableinstances to run it and check the outputs all matched bit-for-bit.