Fix converting Rxx and similar Qiskit gates #2579
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @gluonhiggs, thank you for submitting a PR to Mitiq! We will respond as soon as possible, and if you have any questions in the meantime, you can ask us on the Unitary Fund Discord.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2579 +/- ##
=======================================
Coverage 98.72% 98.72%
=======================================
Files 92 92
Lines 4168 4169 +1
=======================================
+ Hits 4115 4116 +1
Misses 53 53 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -255,6 +255,9 @@ def from_qiskit(circuit: qiskit.QuantumCircuit) -> cirq.Circuit: | |||
| # Try to decompose circuit before running | |||
| # This is necessary for converting qiskit circuits with | |||
| # custom packaged gates, e.g., QFT gates | |||
| circuit = circuit.decompose( | |||
| gates_to_decompose=["u1", "u2", "u3", "cx", "rx", "ry"] | |||
There was a problem hiding this comment.
First, thanks @gluonhiggs for the quick fix!
I am not sure about this line where you are decomposing simple one-qubit gates such as "cx", "rx", "ry". Those gates are supported by Mitiq/Cirq and we risk to over-decompose the circuit.
I understand that tests are passing, but this change may unnecessarily increase the depth of the output circuit and this is a problem for the efficiency of the quantum computation. For example, if you input a short qiskit circuit made by a single "rzz" gate and many "cx", "rx", "ry" , Mitiq would decompose all gates producing an extremely long output circuit.
I don't know if it works, but I would suggest,it after the existing line circuit=circuit.decompose(),
to add something like this:
circuit = circuit.decompose(gates_to_decompose=["rzz", "rzx", "rxx"], reps=10)
In this way, Mitiq will first apply a general first decomposition step as usual.
Afterwards, if there are still "rzz", "rzx", "rxx" gates left in the circuit, it will simplify only those specific gates.
What do you think?
There was a problem hiding this comment.
For issues such as #2578 and #558, we should just find the maximum set of gates that are supported by both Qiskit's QASM2 serializer and Cirq's QASM2 parser, and transpile to that set. @andreamari wdyt?
There was a problem hiding this comment.
@cosenal I thought about that and tried to transpile not to the maximum set of the common gates, but to the set of the gates that can be used to construct the others, i.e., ["u1", "u2", "u3", "cx", "rx", "ry", "rz"], then decompose(). I also thought that this would be efficient because transpile comes with optimization_method. However, the same error still happens. I even tried with a broader set, e.g., ["u3", "u2", "u1", "cx", "id", "x", "y", "z", "h", "s", "t", "rx", "ry", "rz"], but nothing has changed. Generally, I think it would be hard to determine the set because there will be more n-qubit gates, right?
@andreamari If we choose the set gate to decompose like ["rzz", "rzx", "rxx"], I'm wonder if we also have to consider other combinations of x, y, and z here like ryy, ryz, and so on? Because I tried to replace rzz by ryy in the circuit (in the case ["rzz", "rzx", "rxx"]), there will be an error.
test_qc1 = QuantumCircuit(2)
test_qc1.rx(0.1, 0)
test_qc1.ryy(0.1, 0, 1)
print(test_qc1)
print(convert_to_mitiq(test_qc1))And i think over decompose will happen if we add more rii gates (where i in {x, y, z})?
There was a problem hiding this comment.
What I am proposing is something like
circuit = qiskit.transpile(
circuit,
basis_gates=QASM2_GATE_SET,
optimization_level=0,
)
mitiq_circuit = from_qasm(qasm2.dumps(circuit))without the decompose(), because that would bring the gate set to another set that may not be supported by QASM serializer/parser.
I tried with QASM2_GATE_SET = ["u1", "u2", "u3", "cx", "rx", "ry", "rz"] and it seems to work fine. I don't think that's the maximal set though. Having the maximal QASM-supported set there is the best we can do. And something smaller than the maximal will be sub-optimal when it comes to the size of the output circuit (as Andrea pointed out.)
There was a problem hiding this comment.
Yeah. It works. Let me have some tests. @andreamari what do you think? I think we can increase the level of optimization or expose the option to the interface.
Update 1: @cosenal i have tried different variants of this approach, but some tests can't be passed.
Update 2: Because transpile is for transforming a circuit to satisfy specific constraints including hardware, I don't think we should use transpile here. To the best of my knowledge, decompose is just a "pure" break of circuits to some basis gates, and to optimize the decomposition (which is generally a difficult problem, even though we have algorithms to do that for some circuits), we either should choose a good set gates_to_decompose globally optimal (what we got so far is "u1", "u2", "u3", "cx", "rx", "ry"), or we suggest a change on qiskit repo. I'd love to hear your thoughts on this.
There was a problem hiding this comment.
@andreamari I can distinguish gates_to_decompose and basis_gates. Perhaps, the way I expressed my thoughts was not clear at some point. The problem here is for some reasons, when I tried to decompose the "bad gates" (different lists of rii with this reference), the error similar to #2578 (the original issue) still happens.
@cosenal Here is the short test summary from pytest:
================================================================================= short test summary info =================================================================================
FAILED mitiq/interface/mitiq_qiskit/tests/test_conversions_qiskit.py::test_convert_with_qft - AssertionError: assert False
FAILED mitiq/zne/scaling/tests/test_folding.py::test_convert_qiskit_to_mitiq_circuit_conversion_error - Failed: DID NOT RAISE <class 'mitiq.interface.conversions.CircuitConversionError'>
FAILED mitiq/zne/scaling/tests/test_folding.py::test_folding_circuit_conversion_error_qiskit[fold_gates_at_random] - Failed: DID NOT RAISE <class 'mitiq.interface.conversions.CircuitConversionError'>
FAILED mitiq/zne/scaling/tests/test_folding.py::test_folding_circuit_conversion_error_qiskit[fold_global] - Failed: DID NOT RAISE <class 'mitiq.interface.conversions.CircuitConversionError'>Should I paste the whole log here or move the discussion to discord?
Here are the tests failed:
Solution 1:
mitiq/interface/mitiq_qiskit/tests/test_conversions_qiskit.py::test_convert_with_qft
mitiq/zne/tests/test_zne.py::test_execute_zne_on_qiskit_circuit_with_QFT
Solution 2:
mitiq/interface/mitiq_qiskit/tests/test_conversions_qiskit.py::test_convert_with_qft
mitiq/zne/scaling/tests/test_folding.py::test_convert_qiskit_to_mitiq_circuit_conversion_error
mitiq/zne/scaling/tests/test_folding.py::test_folding_circuit_conversion_error_qiskit[fold_gates_at_random]
mitiq/zne/scaling/tests/test_folding.py::test_folding_circuit_conversion_error_qiskit[fold_global]
The error log is too lengthy that I can't even paste to discord.
There was a problem hiding this comment.
@andreamari Why would we need repeated decompose() if we add all gates we don't like in
gates_to_decompose?
I think you are right, if all bad gates are in gates_to_decompose you probably don't need to also keep the existing line circuit.decompose() (without options).
However I think you would still need the reps=... option, since it ensures that potential bad gates that may be created during the decomposition process are further decomposed.
There was a problem hiding this comment.
FYI: I have updated the code and it seems to work fine thanks to your suggestion @andreamari
The story is:
I fell in the 2nd approach that used transpile to transform the circuits to some pre-declared basis_gates. I had tried dozens of combinations of the supported gates to form basis_gates. In the end, I realized that a very good choice of basis_gates passing almost all test cases but still can cause transpile to fail these tests
mitiq/zne/scaling/tests/test_folding.py::test_convert_qiskit_to_mitiq_circuit_conversion_error
mitiq/zne/scaling/tests/test_folding.py::test_folding_circuit_conversion_error_qiskit[fold_gates_at_random]
mitiq/zne/scaling/tests/test_folding.py::test_folding_circuit_conversion_error_qiskit[fold_global]because transpile is too strong that can transform any circuit of both supported and unsupported gates to a circuit constructed from basis_gates. In the context of mitiq, to my understanding, we do not implement the noise mitigation for unsupported gates (or custom gates in the failed test) directly. Therefore, we expect that convert_to_mitiq will raise error in case there are custom gates, instead of transforming the circuit normally. So, this is too much. We should not use transpile here.
Back to the 1st approach, when these tests are failed
mitiq/interface/mitiq_qiskit/tests/test_conversions_qiskit.py::test_convert_with_qft
mitiq/zne/tests/test_zne.py::test_execute_zne_on_qiskit_circuit_with_QFTso I tried to add "QFT" to the gates_to_decompose and it's worked since then. Previously, instead of using "QFT", I used "qft". That's why it didn't work.
There was a problem hiding this comment.
since it ensures that potential bad gates that may be created during the decomposition process
I would be surprised if gates from the gates_to_decompose reappear during decomposition. Something we can ask to Qiskit folks.
There was a problem hiding this comment.
@gluonhiggs if we go with the approach I see now in the PR (good that it works), we could do the decomposition directly instead of doing it as a fallback when the conversion fails.
gluonhiggs
force-pushed
the
resolve-issue2578
branch
2 times, most recently
from
6ab8365 to
4d07521
Compare
cosenal
changed the title
gluonhiggs
force-pushed
the
resolve-issue2578
branch
from
93315c3 to
3b81eb2
Compare
| circuit = circuit.decompose() | ||
| gates_to_decompose = ["rxx", "rzz", "rzx", "ryy", "QFT"] | ||
| circuit = circuit.decompose( | ||
| gates_to_decompose=gates_to_decompose, reps=10 |
There was a problem hiding this comment.
from what I understand of decompose, we shouldn't need reps=10 here, see discussion with @andreamari in the other comment.
| try: | ||
| mitiq_circuit = from_qasm(qasm2.dumps(circuit)) | ||
| except QasmException: | ||
| # Try to decompose circuit before running | ||
| # This is necessary for converting qiskit circuits with | ||
| # custom packaged gates, e.g., QFT gates | ||
| circuit = circuit.decompose() | ||
| gates_to_decompose = ["rxx", "rzz", "rzx", "ryy", "QFT"] |
There was a problem hiding this comment.
nit: this should be capitalized as we want to treat it as a constant
There was a problem hiding this comment.
I have updated the code. Thank you!
Description
Resolves issue #2578
Basically, the error happens technically due to the fact that
rzzis not recognized by QASM.In general, this can also happen to other gates that are pre-built in any framework but not existing in QASM.
So, the idea is that we should
decompose()some parts of the argument circuit such that these gate are more recognizable to QASM, and thendecompose()one more time to obtain the decomposition by the basis gates.My original thought was:
License
Before opening the PR, please ensure you have completed the following where appropriate.
I updated the documentation where relevant.no doc updated.