-
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 parameter handling for NLocal(..., flatten=True)
and standard gates
#13482
Conversation
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 12034807511Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - 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.
I don't think this actually fixes the issue. This is sidestepping the underlying bug by changing the construction of the n local circuit to not initially populate the py gate cache anymore. This means that the params in the cached python gate objects and the what's stored in circuit data/circuit instruction/packed instruction don't go out of sync because there isn't anything cached yet. If you updated the test to read every gate object from the circuit before assigning the params I think this would fail in the same way reported in the issue.
To fix this fully I'm pretty sure you need to update: https://github.com/Qiskit/qiskit/blob/main/crates/circuit/src/circuit_data.rs#L1382-L1389 to update the cached py gate, if the cache has been set.
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 now, this looks the correct fix, thanks for updating.
…ates (#13482) (#13495) * Use _append_standard_gate directly * update params on cache * Revert "Use _append_standard_gate directly" This reverts commit 9769785. * Add test and reno (cherry picked from commit 4e5d9f8) Co-authored-by: Julien Gacon <[email protected]>
This change seems to have broken the cutting addon's CI. I don't understand why yet: Qiskit/qiskit-addon-cutting#714. |
Summary
Fixes #13478.
The fast-path for parameterizing standard gates caused an inconsistent state of the circuit, which is now fixed by using the explicit
circuit._append_standard_gate
method.Details and comments
I'm not 100% sure what exactly the inconsistency was. Another way to fix this issue was to have the Rust-side
assign_parameter_inner
to explicitly re-assign theparams
attribute of the cached Python gate. So I'm assuming the old path didn't correctly register the gate in the cache, but I couldn't pin point the exact location in the code. If someone has more insight that would be great to know 🙂