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

Restrict Split2QUnitaries to run on UnitaryGates only #13095

Merged
merged 1 commit into from
Sep 5, 2024

Conversation

Cryoris
Copy link
Contributor

@Cryoris Cryoris commented Sep 5, 2024

Summary

Fixes the remaining problems with #12970.

Details and comments

If we run Split2QUnitaries with an incomplete 1-q basis gate set, we can end up in the situation where a supported gate is split into 2 UnitaryGate objects that might not be representable in the current basis gate set. For example

from qiskit import QuantumCircuit, transpile
from qiskit.circuit.library import *

gate = PauliGate("XX")

qc = QuantumCircuit(2)
qc.append(gate, [0, 1])

basis_gates = ["x"]  # gate decomposes into XX
 
circuit = transpile(qc, basis_gates=basis_gates, optimization_level=2)  # fails -- cannot map U to the basis

This PR fixes the behavior by only running on UnitaryGate objects -- hence not changing the gate types existing in the circuit.

@Cryoris Cryoris 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 labels Sep 5, 2024
@Cryoris Cryoris added this to the 1.2.1 milestone Sep 5, 2024
@Cryoris Cryoris requested a review from a team as a code owner September 5, 2024 15:07
@qiskit-bot
Copy link
Collaborator

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

  • @Qiskit/terra-core

@coveralls
Copy link

Pull Request Test Coverage Report for Build 10723089932

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • 15 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.02%) to 89.17%

Files with Coverage Reduction New Missed Lines %
crates/circuit/src/dag_node.rs 3 81.2%
crates/circuit/src/operations.rs 3 88.5%
crates/qasm2/src/lex.rs 4 91.73%
crates/circuit/src/circuit_instruction.rs 5 85.14%
Totals Coverage Status
Change from base Build 10721003464: 0.02%
Covered Lines: 72657
Relevant Lines: 81481

💛 - 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, I'll make sure to update #13025 to reflect this change too.

This could arguably be considered a behavior change but I think we're in the grey area because the pass was always named Split2QUnitaries (I totally missed the capital Q which is lowercase for everything else) and the intent was always for this to be run after consolidate blocks. Also since this is actively breaking use cases that worked in < 1.2 I would treat this as the lesser change.

@mtreinish mtreinish enabled auto-merge September 5, 2024 15:43
@mtreinish mtreinish added this pull request to the merge queue Sep 5, 2024
Merged via the queue into Qiskit:main with commit 9898979 Sep 5, 2024
15 checks passed
mergify bot pushed a commit that referenced this pull request Sep 5, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 6, 2024
(cherry picked from commit 9898979)

Co-authored-by: Julien Gacon <[email protected]>
mtreinish added a commit to mtreinish/qiskit-core that referenced this pull request Sep 6, 2024
Some of the logic inside the Split2QUnitaries pass was updated in a
recently merged PR. This commit makes those changes so the rust
implementation matches the current state of the previous python version.
github-merge-queue bot pushed a commit that referenced this pull request Sep 9, 2024
* Fully port Split2QUnitaries to rust

This commit builds off of #13013 and the other data model in Rust
infrastructure and migrates the InverseCancellation pass to
operate fully in Rust. The full path of the transpiler pass now never
leaves Rust until it has finished modifying the DAGCircuit. There is
still some python interaction necessary to handle parts of the data
model that are still in Python, mainly for creating `UnitaryGate`
instances and `ParameterExpression` for global phase. But otherwise
the entirety of the pass operates in rust now.

This is just a first pass at the migration here, it moves the pass to
use loops in rust. The next steps here are to look at operating
the pass in parallel. There is no data dependency between the
optimizations being done for different gates so we should be able to
increase the throughput of the pass by leveraging multithreading to
handle each gate in parallel. This commit does not attempt
this though, because of the Python dependency and also the data
structures around gates and the dag aren't really setup for
multithreading yet and there likely will need to be some work to
support that.

Part of #12208

* Update pass logic with changes from #13095

Some of the logic inside the Split2QUnitaries pass was updated in a
recently merged PR. This commit makes those changes so the rust
implementation matches the current state of the previous python version.

* Use op_nodes() instead of topological_op_nodes()

* Use Fn trait instead of FnMut for callback

We don't need the callback to be mutable currently so relax the trait to
just be `Fn` instead of `FnMut`. If we have a need for a mutable
environment callback in the future we can change this easily enough
without any issues.

* Avoid extra edge operations in replace_on_incoming_qubits

* Rename function
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 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