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

qiskit.transpile with optimization_level=3 does not optimize away an identity operation like cp with phase 0 #13344

Closed
dixr opened this issue Oct 18, 2024 · 2 comments · Fixed by #12384
Labels
bug Something isn't working

Comments

@dixr
Copy link

dixr commented Oct 18, 2024

Environment

  • Qiskit version: 1.2.4
  • Python version: 3.11.3
  • Operating system: Ubuntu Linux

What is happening?

Why does qiskit.transpile() function with optimization level 3 not remove a cp (controlled phase) gate when its parameter is 0? Since a cp(0) gate is equivalent to an identity operation, it should be optimized away.

How can we reproduce the issue?

import qiskit
import qiskit_aer
circuit = qiskit.QuantumCircuit(2)
circuit.cp(0, 1, 0)
circuit = qiskit.transpile(circuit, qiskit_aer.AerSimulator(), optimization_level=3)
circuit.data

Output:

[CircuitInstruction(operation=Instruction(name='cp', num_qubits=2, num_clbits=0, params=[0.0]), qubits=(Qubit(QuantumRegister(2, 'q'), 1), Qubit(QuantumRegister(2, 'q'), 0)), clbits=())]

What should happen?

The gate should be removed.

Any suggestions?

No response

@dixr dixr added the bug Something isn't working label Oct 18, 2024
@Cryoris
Copy link
Contributor

Cryoris commented Oct 18, 2024

Short answer: Yeah we currently don't do value-based optimization for gates that are already in the target basis (note that the AerSimulator accepts any standard gate in its basis). To resolve this you could set a specific basis that forces Qiskit to re-synthesize the operation. For example

import qiskit
circuit = qiskit.QuantumCircuit(2)
circuit.cp(0, 1, 0)
circuit = qiskit.transpile(circuit, basis_gates=["u", "cx"], optimization_level=3)  

That should also be in the correct format for the AerSimulator, I think.

Longer answer: This is generally an optimization we miss, here's a more basic example:

import qiskit

circuit = qiskit.QuantumCircuit(1)
circuit.ry(0, 0)
circuit = qiskit.transpile(circuit, basis_gates=["ry"], optimization_level=3, callback=callback)
print(circuit)  # Ry(0) present!

circuit = qiskit.transpile(circuit, basis_gates=["u"], optimization_level=3, callback=callback)
print(circuit)  # empty!

In the second example, the Ry(0) gets converted to U(0,0,0) and subsequently Optimize1qGatesDecomposition removes the gate. We could maybe expand that workflow to remove 0-angle rotations with the same tolerance that it uses in converting Euler angles to the identity 🤔

mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Oct 18, 2024
Right now when we're running the consolidate blocks pass during the init
stage we were not consolidating all 2q blocks found. This was because
the pass was not being called with the target's information and it
wasn't aware when the gates in a block were outside of the target. To
avoid potentially unecessary work the ConsolidateBlocks pass has some
logic to avoid collecting into a block if it doesn't think that the
unitary decomposition can decrease the 2q gate count, basically trying
to avoid the case where the synthesis output would be more expensive
than original circuit gates. However if the gates are outside of the
target it can't estimate the synthesis cost and should be consolidating
the block. The logic for this exists in the pass, but it only works if
you specify the target so it knows that there are gates in the block
outside the target.

As part of this a bug around handling gates outside basis in the
pass was fixed. If gates were encountered that had no matrix defined
the pass would previously have errored trying to create a unitary gate
for the block in some cases.

In general the logic around this pass is lacking in several cases (see
issue Qiskit#11659) and it would be better to estimate the error of the block
and determine if the estimated error from the output of different
synthesis alternatives is better or not. This is easy to do if we do
consolidation and synthesis in a single step. But for this situation of
running consolidation during init stage and synthesis much later during
the translation stage this is hard to accomplish because we lose the
context of the original block if we consolidate. That is why we should
consider changing the init stage's use of `ConsolidateBlocks` to set
`force_consolidate=True` which will create a `UnitaryGate` out of all
2q blocks found regardless of the estimated number of KAK gates needed
based on the unitary matrix's coordinates in the weyl chamber. Doing
this would fix issues like Qiskit#13344 because we'll synthesisze the unitary
in that case. Where it's less clear we want to do this though is cases
where the target has different 2q gates than the KAK gate synthesis
knows how to work with. Take the example of Qiskit#13344, if the target had
global supported gates of `[cp, u, cx]` or something like that and the
angle was not zero then forcing consolidation would change a single cp
into possibly 3 cx gates. So this commit does not set
`force_consolidate=True` and we can discuss the best way to handle that
in a separate commit (either on this PR or a new one).
@Cryoris Cryoris linked a pull request Oct 23, 2024 that will close this issue
1 task
@Cryoris
Copy link
Contributor

Cryoris commented Oct 23, 2024

Update: this should get fixed by #12384.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants