Fix random seed initialization between runs in SabreSwap #9127
Conversation
This commit fixes an issue in the case no random seed is provided by the user when initializing an instance of SabreSwap. The pass was previously potentially reusing the random seed between runs even if no seed was specified. This was an artifact of storing the initial seed as an instance variable. Instead this commit just relies on the the Rust RNG to initialize from entropy if no seed is specified.
mtreinish
added
stable backport potential
|
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
mtreinish
added
Rust
Pull Request Test Coverage Report for Build 3470354105
💛 - Coveralls |
There was a problem hiding this comment.
I'm not 100% convinced that this is a bug, to be honest - we don't really specify what should happen in the documentation, and given there's a non-zero cost to instantiating a SabreSwap object (technically the cached coupling_map.distance_matrix call is the asymptotically limiting component of the algorithm), it's conceivable that even somebody fixing the initial seed may want to reuse the same object multiple times, and have the RNG continue over multiple runs for different circuits. That said, I'm not certain how well that idea plays with how our parallelisation works.
I recognise that this patch restores the behaviour of the non-Rust implementations, so I'm happy to accept it on those grounds, but I do wonder if that is necessarily the best choice.
|
The parallelization piece of this was actually why I pushed this. Running: #9090 (comment) without this PR applied returns 68 for each of the 20 circuits. I was debugging #9116 and came across this behavior was caused by the same seed was used for all the process workers. I pushed this to restore the spread across the multiple calls. If people weren't using |
* Fix random seed initialization between runs in SabreSwap This commit fixes an issue in the case no random seed is provided by the user when initializing an instance of SabreSwap. The pass was previously potentially reusing the random seed between runs even if no seed was specified. This was an artifact of storing the initial seed as an instance variable. Instead this commit just relies on the the Rust RNG to initialize from entropy if no seed is specified. * Remove unused numpy import (cherry picked from commit dfbc738)
) (#9132) * Fix random seed initialization between runs in SabreSwap (#9127) * Fix random seed initialization between runs in SabreSwap This commit fixes an issue in the case no random seed is provided by the user when initializing an instance of SabreSwap. The pass was previously potentially reusing the random seed between runs even if no seed was specified. This was an artifact of storing the initial seed as an instance variable. Instead this commit just relies on the the Rust RNG to initialize from entropy if no seed is specified. * Remove unused numpy import (cherry picked from commit dfbc738) * Restore numpy import On the main branch the SabreSwap python code has been refactored so it no longer is using numpy besides the previous use for the RNG. Removing the numpy RNG usage in this PR meant also removing the numpy import as nothing else was using it anymore. However, the version of SabreSwap in stable/0.22 does not have this refactor and numpy is still used to construct intermediate objects which are passed to rhe rust code. This commit restores the import to fix the backported PR so it works with the version of SabreSwap in stable/0.22 Co-authored-by: Matthew Treinish <mtreinish@kortar.org>
* Fix random seed initialization between runs in SabreSwap This commit fixes an issue in the case no random seed is provided by the user when initializing an instance of SabreSwap. The pass was previously potentially reusing the random seed between runs even if no seed was specified. This was an artifact of storing the initial seed as an instance variable. Instead this commit just relies on the the Rust RNG to initialize from entropy if no seed is specified. * Remove unused numpy import
Summary
This commit fixes an issue in the case no random seed is provided by the user when initializing an instance of SabreSwap. The pass was previously potentially reusing the random seed between runs even if no seed was specified. This was an artifact of storing the initial seed as an instance variable. Instead this commit just relies on the the Rust RNG to initialize from entropy if no seed is specified.
Details and comments