Skip to content

Conversation

MozammilQ
Copy link
Contributor

@MozammilQ MozammilQ commented May 18, 2024

Summary

This PR aims to fix an issue withTarget.has_calibration() and Target.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 as args, the method tried to bind this args with a parameter in the schedule, which was not the intention of this particular args.

So, the signature of Target.has_calibration includes a new argument operation_params which has to be passed with the parameter of the Gate if the Gate is parameterized.

Similarly, the signature of Target.get_calibration also includes a new argument operation_params which has to be passed with the parameter of the Gate if the Gate is parameterized. In the case of args 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

@MozammilQ MozammilQ requested a review from a team as a code owner May 18, 2024 01:41
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label May 18, 2024
@qiskit-bot
Copy link
Collaborator

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:

  • @Qiskit/terra-core
  • @nkanazawa1989

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9136334258

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 2 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.006%) to 89.607%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 92.62%
Totals Coverage Status
Change from base Build 9134495129: 0.006%
Covered Lines: 62302
Relevant Lines: 69528

💛 - Coveralls

@MozammilQ MozammilQ changed the title [WIP] Check for gate parameter Check for gate parameter May 19, 2024
@MozammilQ
Copy link
Contributor Author

@1ucian0 , pulse features are being deprecated in Qiskit, and being moved to Qiskit Dynamics, is this PR required anymore?
If yes, in what direction the PR should be heading?

Copy link
Member

@eliarbel eliarbel left a 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:

  1. Update your branch to include all the recent changes including the move Target to Rust. We still have Target class in target.py with has_calibration and get_calibration there so your changes are still valid.
  2. 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.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 11995012388

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • 16 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.009%) to 88.945%

Files with Coverage Reduction New Missed Lines %
crates/qasm2/src/lex.rs 4 93.23%
crates/qasm2/src/parse.rs 12 97.15%
Totals Coverage Status
Change from base Build 11977726745: 0.009%
Covered Lines: 79415
Relevant Lines: 89286

💛 - Coveralls

@MozammilQ MozammilQ changed the base branch from main to stable/1.3 November 24, 2024 11:01
@MozammilQ MozammilQ marked this pull request as draft November 24, 2024 11:07
@MozammilQ MozammilQ marked this pull request as ready for review November 25, 2024 23:17
@qiskit-bot
Copy link
Collaborator

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:

  • @Qiskit/terra-core
  • @nkanazawa1989

@MozammilQ
Copy link
Contributor Author

@eliarbel , please have a look :)

@MozammilQ MozammilQ marked this pull request as draft December 7, 2024 06:42
@MozammilQ MozammilQ changed the base branch from stable/1.3 to stable/1.4 December 7, 2024 06:42
@MozammilQ MozammilQ requested a review from eliarbel December 7, 2024 06:43
@MozammilQ MozammilQ marked this pull request as ready for review December 7, 2024 06:43
@qiskit-bot
Copy link
Collaborator

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:

  • @Qiskit/terra-core
  • @nkanazawa1989

Copy link
Member

@eliarbel eliarbel left a 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.

@MozammilQ MozammilQ requested a review from eliarbel December 15, 2024 10:56
Copy link
Member

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @MozammilQ!

@eliarbel eliarbel added this pull request to the merge queue Dec 16, 2024
Merged via the queue into Qiskit:stable/1.4 with commit f13673b Dec 16, 2024
16 checks passed
@ElePT ElePT added the Changelog: Bugfix Include in the "Fixed" section of the changelog label Feb 20, 2025
@ElePT ElePT added this to the 1.4.0 milestone Feb 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: Bugfix Include in the "Fixed" section of the changelog Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

target.has_calibration takes no params, but target.get_calibration does target.get_calibration fails for entries with a fixed parameter
5 participants