-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Track QuantumCircuit.global_phase
in ParameterTable
#11428
Track QuantumCircuit.global_phase
in ParameterTable
#11428
Conversation
One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 7280677428Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
e446589
to
ac4fd74
Compare
We have previously always had a split where circuit parameters used in instructions were tracked in the `ParameterTable`, but any parameters used in the global phase were not. Any method that influenced the parameters needed to separately check the global phase, and merge that information with that in the `ParameterTable`. This made it easy to forget, or easy for the handling of it to become out of sync. This commit now tracks the global phase as part of the `ParameterTable`, so this object is now the canonical source of parameter information for the circuit (outside the context of calibrations, which are handled entirely separately). The `ParameterTable` is an internal detail, and only accessible through private attributes, so is not part of the public interface.
ac4fd74
to
bbe7fc0
Compare
Now rebased over #11109. |
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.
From my (less limited than a few hours ago, but still limited) understanding, I don't see any functional issue with this implementation. It might not be the most straightforward, manual intervention is needed in a few points to make sure that the global phase is tracked properly (for example in copy
), and this is adding responsibility to the developers (to keep on top of these details). But at the same time, I cannot come up with any concrete feedback or suggestions to improve it. I will approve to make sure this PR is not blocked, but it might be worth having somebody else give it some thought too.
Yeah, that's pretty much how I felt writing it as well, haha. I don't like what I've done hugely, but I don't have any better ideas, and overall this makes parameter-handling code more local. |
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.
I'll need to update #11214 once this merges to handle parameter remapping in circuit_to_instruction
.
Approving but holding off on merge in case addressing my comment warrants any code change. Feel free to merge as is.
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, thanks!
Summary
We have previously always had a split where circuit parameters used in instructions were tracked in the
ParameterTable
, but any parameters used in the global phase were not. Any method that influenced the parameters needed to separately check the global phase, and merge that information with that in theParameterTable
. This made it easy to forget, or easy for the handling of it to become out of sync.This commit now tracks the global phase as part of the
ParameterTable
, so this object is now the canonical source of parameter information for the circuit (outside the context of calibrations, which are handled entirely separately). TheParameterTable
is an internal detail, and only accessible through private attributes, so is not part of the public interface.Details and comments
Dependent on #11109, which this PR currently includes. I'll rebase it after that merges, but I'm putting this up a little early just to have things to link to in the dependency chain of #7107.
No changelog because it's a dev-only internal detail (in theory).