-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update gradient logic for Qiskit Rust circuit data implementation #188
Conversation
While this would be mergeable, as the tests against main, though they fail do not prevent things from being merged. I labelled this PR The remaining problem, i.e.this failure
ends up being around the Rust implementation and from these notes, that talk about potential issues, https://github.com/Qiskit/qiskit/blob/main/releasenotes/notes/circuit-gates-rust-5c6ab6c58f7fd2c9.yaml#L47-L79 I added a workaround that simply assigns the current circuit |
Pull Request Test Coverage Report for Build 10177248383Details
💛 - 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.
Thanks a lot @woodsp-ibm for keeping the code updated. I was planning to eventually look into it but you got there faster :)
The last change, to file gradients/utils.py, is not needed with this PR Qiskit/qiskit/pull/12794, so once this merges to Qiskst main I will remove the workaround from that (i,e the data copy) |
Qiskit/qiskit#12794 (Jake's PR) fixes parts of this PR, as it removes the need for copying So we can either
With Qiskit 1.2 imminent maybe option 2 is better? |
Use public API in `derive_circuit` only
@Cryoris Well it seems that things need a little fixup having merged your PR. A couple of copyright dates and mypy typing by the looks. @ElePT It seems with main that more stuff is broke - maybe with 1.2 as well. This PR Qiskit/qiskit#12824 changed base types so computeuncompute now fails e.g.
|
Copyright & mypy done in 0c99ccb |
Sampler base class change reverted in Qiskit/qiskit#12871 |
With Qiskit/qiskit#12794 having been merged earlier today I re-ran the CI and it all passes now. So it looks like this is good to go. |
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 but having touched the code maybe @ElePT should approve too 😄
Co-authored-by: Julien Gacon <jules.gacon@googlemail.com> (cherry picked from commit 23f3b85)
…skit-community#188) Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
Summary
Fixes #181
This updates internals of the gradients which more directly manipulated circuit data the way it was when it was managed in Python. The Rust implementation is changing things to be more performant but things are also different so this changes the logic to try accommodate as things are currently done, which is what is in Qiskit stable releases at present, and the new Rust implementation which is presently in main branch of Qsikit repo. #181 notes the aspect of CI which is failing is when we test against main which was done to monitor for any problems here due to changes.
This has partially fixed things but there is another aspect, that I failed to notice/note in #181 that also occurs
which has not been addressed at present.
Details and comments