-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Check for gate parameter #12439
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
Check for gate parameter #12439
Conversation
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 following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 9136334258Details
💛 - Coveralls |
@1ucian0 , pulse features are being deprecated in Qiskit, and being moved to Qiskit Dynamics, is this PR required anymore? |
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.
Hi @MozammilQ , thanks for working on this! Yes, although Pulse is being deprecated in Qiskit 1.3 and will be removed in 2.0, it would still be good to have this as part of the extended support window for the 1.x series.
For moving forward please note the following:
- Update your branch to include all the recent changes including the move
Target
to Rust. We still haveTarget
class intarget.py
withhas_calibration
andget_calibration
there so your changes are still valid. - Once this PR is ready we will want to merge it into the 1.4 branch and not 2.0.
I also left some line comments after partially reviewing this.
releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml
Outdated
Show resolved
Hide resolved
Pull Request Test Coverage Report for Build 11995012388Details
💛 - Coveralls |
ddc9411
to
6aa011f
Compare
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 following people are relevant to this code:
|
@eliarbel , please have a look :) |
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 following people are relevant to this code:
|
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.
Looks good overall. A few more minor changes and this should be good to go.
releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml
Outdated
Show resolved
Hide resolved
releasenotes/notes/Fixed-param-gate-target-has-and-get_calibration-d2b9e38d63592b3c.yaml
Outdated
Show resolved
Hide resolved
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 @MozammilQ!
Summary
This PR aims to fix an issue with
Target.has_calibration()
andTarget.get_calibration()
when passed with parameterized gates didn't work as expected.Refer to issues for more information.
#11658
#11657
Details and comments
To be precise,
Target.has_calibration()
didn't check if the calibration if present is present for a particular gate with a particular parameter,and,
Target.get_calibration()
didn't get the calibration for the gate if present is present for a particular gate with a particular parameter, when passed as a parameter asargs
, the method tried to bind thisargs
with a parameter in the schedule, which was not the intention of this particularargs
.So, the signature of
Target.has_calibration
includes a new argumentoperation_params
which has to be passed with the parameter of theGate
if the Gate is parameterized.Similarly, the signature of
Target.get_calibration
also includes a new argumentoperation_params
which has to be passed with the parameter of theGate
if the Gate is parameterized. In the case ofargs
argument in this function, it should be passed with the values to be bound with the parameter of the schedule which has been attached to this particular gate as its calibration.fixes #11658
fixes #11657