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.

Sign up for GitHub

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

Jump to bottom

Unitaries equal to identity are not removed from circuit at optimization_level=3 #9217

Closed
nonhermitian opened this issue Nov 30, 2022 · 3 comments · Fixed by #9222
Closed

Unitaries equal to identity are not removed from circuit at optimization_level=3 #9217

nonhermitian opened this issue Nov 30, 2022 · 3 comments · Fixed by #9222
Assignees
@mtreinish
Labels
bug Something isn't working

Comments

@nonhermitian
Copy link
Contributor

Environment

  • Qiskit Terra version: latest
  • Python version:
  • Operating system:

What is happening?

The following circuit is equal to the identity:

qr1 = QuantumRegister(3, 'state')
qr2 = QuantumRegister(2, 'ancilla')
cr = ClassicalRegister(2, 'c')

qc2 = QuantumCircuit(qr1, qr2, cr)
qc2.h(qr1[0])
qc2.cx(qr1[0], qr1[1])
qc2.cx(qr1[1], qr1[2])

qc2.cx(qr1[1], qr1[2])
qc2.cx(qr1[0], qr1[1])
qc2.h(qr1[0])

At optimization_level=2 one gets a empty circuit:

image

At optimization_level=3 one does not:

image

What is left is a unitary that is equal to the identity (as it should be) but it is not removed from the circuit data.

How can we reproduce the issue?

Transpile above example

What should happen?

The circuit should be empty like at optimization_level=2

Any suggestions?

No response
@nonhermitian nonhermitian added the bug Something isn't working label Nov 30, 2022
@mtreinish
Copy link
Member

So I think the issue here is that you're not specifying a target or basis_gates in the transpile call. The 2q unitary synth optimization passes need a basis to determine how to synthesize the unitary and without that being specified it's skipping the synthesis stage after the circuit is collected into a single unitary block.

That being said it's probably not too much extra overhead to add a check either in the collection pass or in the synthesis pass that still checks for identity unitary removal even without a target specified. As this isn't the first time this exact behavior has been raised in an issue.

@nonhermitian nonhermitian changed the title Unitaries not equal to identity are not removed from circuit at optimization_level=3 Unitaries equal to identity are not removed from circuit at optimization_level=3 Nov 30, 2022
@nonhermitian
Copy link
Contributor Author

The difference in behavior between optimization levels is the surprising part, and leaves one having to explain why the higher optimization level is also not an empty circuit; A tricky conversation when it is an implementation issue, and one that until now I was not even aware of.

@mtreinish mtreinish self-assigned this Nov 30, 2022
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Nov 30, 2022
@mtreinish
Enable simplifying 1q runs and 2q blocks in transpile() without target
d05ed0c 
This commit fixes an issue when transpile() was called with optimization
enabled (optimization levels 1, 2, and 3) and no target (or basis gates)
specified then 1q run or 2q block optimization was run. This would result
in long series of gates that could be simplified being left in the output
circuit. To address this, for 1q optimization the
Optimize1qGatesDecomposition pass (which is run at all 3 optimization
levels) is updated so that if no target is specified we just select the
UGate decomposer as the default heuristic for the best output is the
shortest sequence length (in the absense of error rates from the target)
and if any output gate is valid that will either remove the 1q sequence
if it's an identity sequence, or be a single gate. As no basis is
specified this behavior is fine since the calculations.

For optimization level 3 with it's 2q blcok optimization with the
UnitarySynthesis pass it is a bit more involved though. The cost of
doing the unitary synthesis is higher, the number of possible
decompositions is larger, and we don't have a good heuristic measure
of which would perform best without a target specified and it's not
feasible to just try all supported basis by the synthesis module.
This means for a non-identity 2 qubit block the output will be a
UnitaryGate (which without a target specified is a valid output).
However, to address the case when an identity block is present in
the circuit this can be removed with very little overhead. To accomplish
this the ConsolidateBlocks pass is updated to check if an identified
2 qubit block is equal the identity matrix and if so will remove that
block from the circuit.

Fixes Qiskit#9217
mtreinish added a commit to mtreinish/qiskit-core that referenced this issue Dec 1, 2022
@mtreinish
Enable simplifying 1q runs and 2q blocks in transpile() without target
1470637 
This commit fixes an issue when transpile() was called with optimization
enabled (optimization levels 1, 2, and 3) and no target (or basis gates)
specified then 1q run or 2q block optimization was run. This would result
in long series of gates that could be simplified being left in the output
circuit. To address this, for 1q optimization the
Optimize1qGatesDecomposition pass (which is run at all 3 optimization
levels) is updated so that if no target is specified we just try all
decomposers as the default heuristic for the best output is the
shortest sequence length (in the absense of error rates from the target)
and if any output gate is valid that will either remove the 1q sequence
if it's an identity sequence, or likely be a single gate. As no basis is
specified this behavior is fine since the calculations are quick and
any output basis will match the constraints the user provided the
transpiler.

For optimization level 3 with it's 2q blcok optimization with the
UnitarySynthesis pass it is a bit more involved though. The cost of
doing the unitary synthesis is higher, the number of possible
decompositions is larger, and we don't have a good heuristic measure
of which would perform best without a target specified and it's not
feasible to just try all supported basis by the synthesis module.
This means for a non-identity 2 qubit block the output will be a
UnitaryGate (which without a target specified is a valid output).
However, to address the case when an identity block is present in
the circuit this can be removed with very little overhead. To accomplish
this the ConsolidateBlocks pass is updated to check if an identified
2 qubit block is equal the identity matrix and if so will remove that
block from the circuit.

Fixes Qiskit#9217
@mtreinish mtreinish mentioned this issue Dec 1, 2022
Merged
@mtreinish
Copy link
Member

#9222 should fix this issue, for 1q optimization it will try all euler basis the decomposer supports and use the result which results in the shortest 1q sequence. For 2q optimization (which is optimization level3 only) it will simplify identity unitary matricies and otherwise just add a UnitaryGate to the circuit if it's not an identity.

@mergify mergify bot closed this as completed in #9222 Dec 1, 2022
mergify bot added a commit that referenced this issue Dec 1, 2022
@mtreinish @mergify
Enable simplifying 1q runs and 2q blocks in transpile() without target (
fd85047 
#9222)

* Enable simplifying 1q runs and 2q blocks in transpile() without target

This commit fixes an issue when transpile() was called with optimization
enabled (optimization levels 1, 2, and 3) and no target (or basis gates)
specified then 1q run or 2q block optimization was run. This would result
in long series of gates that could be simplified being left in the output
circuit. To address this, for 1q optimization the
Optimize1qGatesDecomposition pass (which is run at all 3 optimization
levels) is updated so that if no target is specified we just try all
decomposers as the default heuristic for the best output is the
shortest sequence length (in the absense of error rates from the target)
and if any output gate is valid that will either remove the 1q sequence
if it's an identity sequence, or likely be a single gate. As no basis is
specified this behavior is fine since the calculations are quick and
any output basis will match the constraints the user provided the
transpiler.

For optimization level 3 with it's 2q blcok optimization with the
UnitarySynthesis pass it is a bit more involved though. The cost of
doing the unitary synthesis is higher, the number of possible
decompositions is larger, and we don't have a good heuristic measure
of which would perform best without a target specified and it's not
feasible to just try all supported basis by the synthesis module.
This means for a non-identity 2 qubit block the output will be a
UnitaryGate (which without a target specified is a valid output).
However, to address the case when an identity block is present in
the circuit this can be removed with very little overhead. To accomplish
this the ConsolidateBlocks pass is updated to check if an identified
2 qubit block is equal the identity matrix and if so will remove that
block from the circuit.

Fixes #9217

* Fix docs build

* Fix handling of 1q decomposer logic with no basis gates

This commit fixes an oversight in the previous commit for the 1q
decomposer pass when no basis gates are specified. Previously we were
setting the basis list to be the set of all the gates in the supported
euler basis for the decomposer. This had the unintended side effect of
breaking the heuristic for the decomposer as it would treat any 1q gate
outside of the supported euler basis as needing to be translated. This
was not the desired behavior as any gate is valid. This fixes the logic
to just treat no basis explicitly as everything being valid output and
weight the heuristic error only.

* Remove debug prints

* Remove unnecessary conditional 1q identity matrix creation

* Split out release notes for 1q and 2q cases

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Cryoris pushed a commit to Cryoris/qiskit-terra that referenced this issue Jan 12, 2023
@mtreinish @mergify @Cryoris
Enable simplifying 1q runs and 2q blocks in transpile() without target (
4cb1d18 
Qiskit#9222)

* Enable simplifying 1q runs and 2q blocks in transpile() without target

This commit fixes an issue when transpile() was called with optimization
enabled (optimization levels 1, 2, and 3) and no target (or basis gates)
specified then 1q run or 2q block optimization was run. This would result
in long series of gates that could be simplified being left in the output
circuit. To address this, for 1q optimization the
Optimize1qGatesDecomposition pass (which is run at all 3 optimization
levels) is updated so that if no target is specified we just try all
decomposers as the default heuristic for the best output is the
shortest sequence length (in the absense of error rates from the target)
and if any output gate is valid that will either remove the 1q sequence
if it's an identity sequence, or likely be a single gate. As no basis is
specified this behavior is fine since the calculations are quick and
any output basis will match the constraints the user provided the
transpiler.

For optimization level 3 with it's 2q blcok optimization with the
UnitarySynthesis pass it is a bit more involved though. The cost of
doing the unitary synthesis is higher, the number of possible
decompositions is larger, and we don't have a good heuristic measure
of which would perform best without a target specified and it's not
feasible to just try all supported basis by the synthesis module.
This means for a non-identity 2 qubit block the output will be a
UnitaryGate (which without a target specified is a valid output).
However, to address the case when an identity block is present in
the circuit this can be removed with very little overhead. To accomplish
this the ConsolidateBlocks pass is updated to check if an identified
2 qubit block is equal the identity matrix and if so will remove that
block from the circuit.

Fixes Qiskit#9217

* Fix docs build

* Fix handling of 1q decomposer logic with no basis gates

This commit fixes an oversight in the previous commit for the 1q
decomposer pass when no basis gates are specified. Previously we were
setting the basis list to be the set of all the gates in the supported
euler basis for the decomposer. This had the unintended side effect of
breaking the heuristic for the decomposer as it would treat any 1q gate
outside of the supported euler basis as needing to be translated. This
was not the desired behavior as any gate is valid. This fixes the logic
to just treat no basis explicitly as everything being valid output and
weight the heuristic error only.

* Remove debug prints

* Remove unnecessary conditional 1q identity matrix creation

* Split out release notes for 1q and 2q cases

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mtreinish mtreinish

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Enable simplifying 1q runs and 2q blocks in transpile() without target mtreinish/qiskit-core
2 participants
@nonhermitian @mtreinish