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 extraction of controlled parametric gates #13067

Merged
merged 1 commit into from
Sep 3, 2024

Conversation

jakelishman
Copy link
Member

Summary

The mutable check in the controlled-gate OperationFromPython extraction logic to check for a mutated base_gate was overzealous, and would return false positives for parametric controlled gates. The only modification to base_gate of a standard-library gate that would not have caused data-model problems from Python space would be setting the base-gate label, which is used for a public feature of the circuit visualisers.

The change to get_standard_gate_name_mapping is just a minor convenience to make the gate objects directly appendable to a circuit; previously, each Parameter object was distinct and had a UUID clash with others of the same name, so could not be used together. The new behaviour is purely a convenience for tests; it largely should not be useful for users to directly append these gates.

Details and comments

The `mutable` check in the controlled-gate `OperationFromPython`
extraction logic to check for a mutated `base_gate` was overzealous,
and would return false positives for parametric controlled gates.  The
only modification to `base_gate` of a standard-library gate that would
not have caused data-model problems from Python space would be setting
the base-gate label, which is used for a public feature of the circuit
visualisers.

The change to `get_standard_gate_name_mapping` is just a minor
convenience to make the gate objects directly appendable to a circuit;
previously, each `Parameter` object was distinct and had a UUID clash
with others of the same name, so could not be used together.  The new
behaviour is purely a convenience for tests; it largely should not be
useful for users to directly append these gates.
@jakelishman jakelishman added stable backport potential The bug might be minimal and/or import enough to be port to stable Changelog: Bugfix Include in the "Fixed" section of the changelog Rust This PR or issue is related to Rust code in the repository mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Sep 2, 2024
@jakelishman jakelishman added this to the 1.2.1 milestone Sep 2, 2024
@jakelishman jakelishman requested a review from a team as a code owner September 2, 2024 10:21
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Cryoris
  • @Qiskit/terra-core
  • @ajavadia
  • @kevinhartman
  • @mtreinish

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10665329212

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • 22 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.01%) to 89.134%

Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/unitary_synthesis.py 2 88.43%
crates/qasm2/src/lex.rs 4 92.48%
crates/circuit/src/packed_instruction.rs 4 95.19%
crates/circuit/src/dag_circuit.rs 6 88.63%
qiskit/synthesis/two_qubit/xx_decompose/decomposer.py 6 90.84%
Totals Coverage Status
Change from base Build 10640280384: -0.01%
Covered Lines: 71824
Relevant Lines: 80580

💛 - Coveralls

Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

LGTM, this is a straightforward fix and good to see. This actually fixes some edge cases I was hitting during the development of the DAGCircuit PR around things like CXGate ended up as a PyGate in rust incorrectly and complicating equality checks.

@mtreinish mtreinish added this pull request to the merge queue Sep 3, 2024
Merged via the queue into Qiskit:main with commit 8f33084 Sep 3, 2024
15 checks passed
@jakelishman jakelishman deleted the extract-parametric-controlled branch September 3, 2024 15:02
mergify bot pushed a commit that referenced this pull request Sep 3, 2024
The `mutable` check in the controlled-gate `OperationFromPython`
extraction logic to check for a mutated `base_gate` was overzealous,
and would return false positives for parametric controlled gates.  The
only modification to `base_gate` of a standard-library gate that would
not have caused data-model problems from Python space would be setting
the base-gate label, which is used for a public feature of the circuit
visualisers.

The change to `get_standard_gate_name_mapping` is just a minor
convenience to make the gate objects directly appendable to a circuit;
previously, each `Parameter` object was distinct and had a UUID clash
with others of the same name, so could not be used together.  The new
behaviour is purely a convenience for tests; it largely should not be
useful for users to directly append these gates.

(cherry picked from commit 8f33084)
github-merge-queue bot pushed a commit that referenced this pull request Sep 3, 2024
The `mutable` check in the controlled-gate `OperationFromPython`
extraction logic to check for a mutated `base_gate` was overzealous,
and would return false positives for parametric controlled gates.  The
only modification to `base_gate` of a standard-library gate that would
not have caused data-model problems from Python space would be setting
the base-gate label, which is used for a public feature of the circuit
visualisers.

The change to `get_standard_gate_name_mapping` is just a minor
convenience to make the gate objects directly appendable to a circuit;
previously, each `Parameter` object was distinct and had a UUID clash
with others of the same name, so could not be used together.  The new
behaviour is purely a convenience for tests; it largely should not be
useful for users to directly append these gates.

(cherry picked from commit 8f33084)

Co-authored-by: Jake Lishman <jake.lishman@ibm.com>
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 mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library Rust This PR or issue is related to Rust code in the repository stable backport potential The bug might be minimal and/or import enough to be port to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants