Skip to content
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 is_parameterized() function of controlled gates #13952

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AlbertJP
Copy link
Contributor

@AlbertJP AlbertJP commented Mar 4, 2025

Summary

Controlled gates are not recognised as parameterized by the is_parameterized() function. This causes issues in Aer and Algorithms when using circuits with controlled rotations: Qiskit/qiskit-aer#2323, qiskit-community/qiskit-algorithms#216

Details and comments

The is_parameterized() function reads from the _params variable, which is not set by some classes that override the params property, such as ControlledGate, CUGate, AnnotatedOperation and Initialize.

This pull request fixes is_parameterized(), such that it reads the params variable instead, which corresponds to the parameters of the base gate for controlled gates.

This pull request also adds a unit test and fixes an existing one.

AlbertJP added 2 commits March 3, 2025 17:32
This tests whether gates that take a Parameter argument (or
encapsulate such a gate) return True on is_parameterized().

The existing unit test on parameter values skipped all gates after
SingletonGate, and reported failure on MCMTGate because it needs a
gate argument and a number of target gates instead of floats.
@AlbertJP AlbertJP requested a review from a team as a code owner March 4, 2025 10:12
@qiskit-bot qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Mar 4, 2025
@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

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2025

CLA assistant check
All committers have signed the CLA.

@AlbertJP
Copy link
Contributor Author

AlbertJP commented Mar 4, 2025

The unit tests found another issue, the controlled gates in qiskit_aer.backends.name_mapping cannot be initialised anymore. (these did not get loaded when running tox locally, so I did not spot the issue before creating the PR.)

This is caused by commit doichanj/qiskit-aer@4587384 (pull request Qiskit/qiskit-aer#1995). I think this should be fixed on the Aer end?

@AlbertJP
Copy link
Contributor Author

Some tests still fail as Qiskit/qiskit-aer#2326 is not yet merged.

I could change the unit test to test only a fixed list of subclasses, or read the __module__ attribute of the subclasses to exclude those from Aer - which feels like a quite ugly solution.

@mtreinish
Copy link
Member

Some tests still fail as Qiskit/qiskit-aer#2326 is not yet merged.

I could change the unit test to test only a fixed list of subclasses, or read the __module__ attribute of the subclasses to exclude those from Aer - which feels like a quite ugly solution.

To be honest it's not super hacky, in this case I would view the failures as a testing harness bug. The intent of that test is to validate that all the ControlledGate instances in qiskit are correct. It pulling in other class definitions from other libraries that use qiskit is not the intent there and is not something that the test is intended to validate. I'd say updating the test harness to check that the leaf class being tested originates in qiskit is totally correct in this case.

@coveralls
Copy link

coveralls commented Mar 14, 2025

Pull Request Test Coverage Report for Build 13856089544

Warning: 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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 2375 unchanged lines in 110 files lost coverage.
  • Overall coverage increased (+1.1%) to 88.125%

Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/gate_direction.rs 1 97.14%
crates/qasm2/src/expr.rs 1 94.23%
qiskit/circuit/commutation_checker.py 1 94.12%
qiskit/circuit/library/standard_gates/r.py 1 97.56%
qiskit/circuit/library/standard_gates/rxx.py 1 97.73%
qiskit/circuit/library/standard_gates/rzx.py 1 97.73%
qiskit/circuit/library/standard_gates/u2.py 1 96.77%
qiskit/circuit/parameter.py 1 96.72%
qiskit/compiler/transpiler.py 1 98.61%
qiskit/primitives/utils.py 1 92.86%
Totals Coverage Status
Change from base Build 13649569641: 1.1%
Covered Lines: 72713
Relevant Lines: 82511

💛 - Coveralls

@AlbertJP
Copy link
Contributor Author

I have excluded the classes that start with qiskit_aer and now all tests pass.

It only tests classes that are direct subclasses of ControlledGate, but there are not so many interesting classes that are sub-subclasses, only non-parameterized and deprecated ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community PR PRs from contributors that are not 'members' of the Qiskit repo
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants