Skip to content

Only apply MCMT plugin on MCMTGate#13596

Merged
alexanderivrii merged 5 commits into
Qiskit:mainfrom
Cryoris:fix-mcmt-transpile
Jan 7, 2025
Merged

Only apply MCMT plugin on MCMTGate#13596
alexanderivrii merged 5 commits into
Qiskit:mainfrom
Cryoris:fix-mcmt-transpile

Conversation

@Cryoris

@Cryoris Cryoris commented Dec 23, 2024

Copy link
Copy Markdown
Contributor

Summary

Fixes #13563, by enabling the high-level synthesis plugins of MCMT only for the MCMTGate, not any gate called "mcmt".

Details and comments

While we often let it be the user's responsibility to be mindful with gate names, this is a special case as we provide a circuit called "mcmt". This can cause innocent-looking code to break, so we should add this check at least until the MCMT circuit is removed 🙂

@Cryoris Cryoris added Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library labels Dec 23, 2024
@Cryoris Cryoris added this to the 1.3.2 milestone Dec 23, 2024
@qiskit-bot

qiskit-bot commented Dec 23, 2024

Copy link
Copy Markdown
Collaborator

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

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

@coveralls

coveralls commented Dec 23, 2024 •
edited
Loading

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 12649042437

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

  • 12 of 31 (38.71%) changed or added relevant lines in 1 file are covered.
  • 24 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.004%) to 88.938%
Changes Missing Coverage Covered Lines Changed/Added Lines %
qiskit/transpiler/passes/synthesis/hls_plugins.py 12 31 38.71%
Files with Coverage Reduction New Missed Lines %
qiskit/transpiler/passes/synthesis/hls_plugins.py 1 83.38%
crates/accelerate/src/two_qubit_decompose.rs 1 92.13%
crates/qasm2/src/lex.rs 2 92.23%
qiskit/quantum_info/operators/symplectic/sparse_pauli_op.py 20 94.58%
Totals Coverage Status
Change from base Build 12535289542: 0.004%
Covered Lines: 79443
Relevant Lines: 89324
💛 - Coveralls

alexanderivrii
alexanderivrii previously approved these changes Dec 26, 2024
View reviewed changes

@alexanderivrii alexanderivrii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The fix looks straightforward, LGTM!

I had one tiny off-topic question.

Comment thread test/python/circuit/library/test_mcmt.py
Comment on lines +295 to +296
mcmt = MCMT(gate=gate, num_ctrl_qubits=1, num_target_qubits=1)
circuit.append(mcmt, circuit.qubits) # append the MCMT circuit as gate called "MCMT"

@alexanderivrii alexanderivrii Dec 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A bit off-topic. Here we are appending the quantum circuit mcmt to the quantum circuit circuit, yet the docstring for append states instruction: Operation | CircuitInstruction. The code does work, since a QuantumCircuit defines a to_instruction method, which append tries to use when the instruction is not of type Operation. Would it be slightly cleaner to change to circuit.append(mcmt.to_instruction(), ...), or should we change the typehint for the append method?

@Cryoris Cryoris Jan 6, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Updating the docs for append sounds good, since we use append(QuantumCircuit) all over the place 🤔

ShellyGarion
ShellyGarion reviewed Jan 1, 2025
View reviewed changes
Comment thread qiskit/transpiler/passes/synthesis/hls_plugins.py
alexanderivrii
alexanderivrii reviewed Jan 7, 2025
View reviewed changes
Comment thread test/python/circuit/library/test_mcmt.py
alexanderivrii
alexanderivrii approved these changes Jan 7, 2025
View reviewed changes

@alexanderivrii alexanderivrii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @Cryoris for also updating our included plugins so that they all follow the same convention. And you have taken the art of writing tests to a completely new level -- now they read like a fiction story (unfortunate names, intruder circuits, etc) 😄.

@alexanderivrii alexanderivrii mentioned this pull request Jan 7, 2025
Closed
@alexanderivrii alexanderivrii added this pull request to the merge queue Jan 7, 2025
Merged via the queue into Qiskit:main with commit 0eeba0d Jan 7, 2025
@Cryoris Cryoris deleted the fix-mcmt-transpile branch January 7, 2025 12:13
@Cryoris

Cryoris commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

Haha I'm hoping to provide some joy for people reviewing the code 😛

@Cryoris

Cryoris commented Jan 7, 2025

Copy link
Copy Markdown
Contributor Author

@Mergifyio backport stable/1.3

@mergify

mergify Bot commented Jan 7, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

backport stable/1.3

✅ Backports have been created

Details

mergify Bot pushed a commit that referenced this pull request Jan 7, 2025
@Cryoris @mergify
Only apply MCMT plugin on MCMTGate (#13596)
47b8d58 
* Only apply MCMT plugin on MCMTGate

* add checks for other gates

* fix reno

* use == instead of equiv

(cherry picked from commit 0eeba0d)
@mergify mergify Bot mentioned this pull request Jan 7, 2025
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Jan 7, 2025
@mergify @Cryoris
Only apply MCMT plugin on MCMTGate (#13596) (#13618)
103b779 
* Only apply MCMT plugin on MCMTGate

* add checks for other gates

* fix reno

* use == instead of equiv

(cherry picked from commit 0eeba0d)

Co-authored-by: Julien Gacon <jules.gacon@googlemail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShellyGarion ShellyGarion ShellyGarion left review comments
@alexanderivrii alexanderivrii alexanderivrii approved these changes

Assignees

No one assigned

Labels

Changelog: Fixed Add a "Fixed" entry in the GitHub Release changelog. mod: circuit Related to the core of the `QuantumCircuit` class or the circuit library

Projects

None yet

Milestone

1.3.2

Development

Successfully merging this pull request may close these issues.

Error transpiling circuit containing MCMT gate

5 participants

@Cryoris @qiskit-bot @coveralls @alexanderivrii @ShellyGarion