Resolve issue 1073 handle unsupported gates

[gluonhiggs]

Resolve #1073

Description

I have changed the logic in from_qiskit as follows:

1. We attempt to convert the input qiskit circuit to mitiq circuit.
2. If we can't do the 1st step, in case there are these gates ["rxx", "rzz", "rzx", "ryy", "QFT"], we decompose the these gates first using decompose()with reps=10, to ensure there will not be the same gates after the decompositions (because in fact, there will be if we only decompose once).
3. The listed gates in the original statement of Some Qiskit gates are not supported (during the internal conversion to Cirq) #1073 can't be decomposed like in the 2nd steps, so we use PassManager (refer here.

Reason

• Attempt 1: I thought this issue is similar to Decomposition problem with Qiskit circuits #2578 resolved by Fix converting Rxx and similar Qiskit gates  #2579. In Fix converting Rxx and similar Qiskit gates  #2579, we only need to add the unsupported gates (rxx, ryy, rzz, rzx) that have decomposition problem to gates_to_decompose argument as

GATES_TO_DECOMPOSE = ["rxx", "rzz", "rzx", "ryy", "QFT"]
        circuit = circuit.decompose(
            gates_to_decompose=GATES_TO_DECOMPOSE
        )

So, maybe, we only need to add sx, u, p, cu1, ecr to GATES_TO_DECOMPOSE. It turned out that these gates are unchanged under the decomposition.

• Attempt 2: As in Fix converting Rxx and similar Qiskit gates  #2579, we (@andreamari @cosenal ) discussed about using transpile to deal with circuits containing unsupported gates, but this approach caused something like "over-transpilation", then failed test_convert_qiskit_to_mitiq_circuit_conversion_error and test_folding_circuit_conversion_error_qiskit (1). Therefore, we didn't choose this approach in Fix converting Rxx and similar Qiskit gates  #2579. However, I have been thinking about mixing this approach with decompose() (a hybrid approach somehow). I tried the hybrid approach to handle sx, u, p, cu1, ecr, e.g.,

 GATES_TO_DECOMPOSE = ["rxx", "rzz", "rzx", "ryy", "QFT"]
        circuit = circuit.decompose(
            gates_to_decompose=GATES_TO_DECOMPOSE
        )
try:
        mitiq_circuit = from_qasm(qasm2.dumps(circuit))
except QasmException:
        circuit = transpile(circuit, base_gates=["u1", "u2", "u3", "cx"])
        mitiq_circuit = from_qasm(qasm2.dumps(circuit))

The idea is that we decompose rij first, then we transpile the circuit left with sx, u, p, cu1, ecr to the basis_gates.
And, many tests passed, the tests (1) failed. "over-transpilation" still happens.

• Attempt 3: Is there any way that we can transpile the circuits partially, I asked, like transpile only sx, u, p, cu1, ecr.
  There is no exact same thing, but there is PassManager. The failed tests (1) happened because we don't want to convert custom gates to mitiq, and we don't want conversions that, as I understand, folding method can't handle the circuit, like ZNE shouldn't be able to applied to custom gates? And PassManager can help this, because it optimize the circuit transformation in hardware-compatible form. And maybe that's what folding verification is about.
  I'm still not entirely clear on this, why do we need these tests, could you please explain? Thank you

• Attempt 4: I asked myself why don't we use PassManager from the beginning instead of using decompose(), e.g.,

def from_qiskit(circuit: qiskit.QuantumCircuit) -> cirq.Circuit:
    """Returns a Mitiq circuit equivalent to the input Qiskit circuit.

    Args:
        circuit: Qiskit circuit to convert to a Mitiq circuit.

    Returns:
        Mitiq circuit representation equivalent to the input Qiskit circuit.
    """
    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
        BASIS_GATES = ["u1", "u2", "u3", "cx"]
        pass_manager = PassManager()
        pass_manager.append(
            BasisTranslator(SessionEquivalenceLibrary, BASIS_GATES)
        )
        circuit = pass_manager.run(circuit)

        # Try converting again
        mitiq_circuit = from_qasm(qasm2.dumps(circuit))
    return mitiq_circuit

I tried that, convert_to_mitiq failed for QFT

• Attempt 5: So, do as what we do in Attempt 4. Write test. During the attempts, I tried many different tests already. I couldn't find any issue with sx, but I still added the corresponding test. I added tests for u, p, cu1, ecr. There is no issue. I created new circuits with combination of these ["rxx", "rzz", "rzx", "ryy", "QFT"] and u, p, cu1, ecr for testing. With this

def test_convert_to_mitiq_with_qft_cu1_rzx():
    """
    Tests that convert_to_mitiq works with QFT, CU1, and RZX gates.
    """
    circuit = qiskit.QuantumCircuit(2)
    circuit.h(0)
    circuit.h(1)
    circuit.append(qiskit.circuit.library.QFT(2), [0, 1])
    circuit.append(qiskit.circuit.library.CU1Gate(np.pi / 3), [0, 1])
    circuit.append(qiskit.circuit.library.RZXGate(np.pi / 4), [0, 1])
    assert convert_to_mitiq(circuit)

Error happened. decompose might still remain QFT (and other similar gates) in the circuit . Therefore, I added reps = 10 to the decompose.

License

• I license this contribution under the terms of the GNU GPL, version 3 and grant Unitary Fund the right to provide additional permissions as described in section 7 of the GNU GPL, version 3.

Before opening the PR, please ensure you have completed the following where appropriate.

• I added unit tests for new code.
• I used type hints in function signatures.
• I used Google-style docstrings for functions.
• I updated the documentation where relevant no doc updated.
• Added myself / the copyright holder to the AUTHORS file

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.73%. Comparing base (7648648) to head (2cb3ee8).
Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2585   +/-   ##
=======================================
  Coverage   98.73%   98.73%           
=======================================
  Files          92       92           
  Lines        4174     4176    +2     
=======================================
+ Hits         4121     4123    +2     
  Misses         53       53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[andreamari]

Great explanation of the situation, thanks @gluonhiggs !
I would like to give some comments/suggestions:

1. If you get any simpler solution in which only test_convert_qiskit_to_mitiq_circuit_conversion_error and test_folding_circuit_conversion_error_qiskit fail, this is ok! I think those tests can be safely removed since they check if, for some non-supported gates say U, an error message is obtained. So, if thanks to this PR, U is now supported, it is ok if no error message is produced.

2. GATES_TO_DECOMPOSE I think should contain as few gates as possible since one never knows if, in the future, some of those gates will be supported by Cirq and so it is difficult to keep up-to-date. E.g., if r_xy gates are correctly transpiled in the next stage, probably, there is no need to manually force their decomposition.

3. BASIS_GATES instead should contain as many gates as possible. This is to avoid over-transpilation into a very limited gateset. For example, with current code, if a Hadamard gate reaches the final transpilation stage, it gets unnecessarily decomposed into "u1", "u2", "u3". To know all supported gates, one could have a look at this part of Cirq code: https://github.com/quantumlib/Cirq/blob/d760f00eb8f277314962c681ebb789476b7e4dd5/cirq-core/cirq/contrib/qasm_import/_parser.py#L185

[gluonhiggs]

Thank you for the information! I think test_folding_circuit_conversion_error_qiskit had some deeper implication than that, because I still don't fully understand "folding" in this context.
If it is as you said, things would be much simpler when we remove the tests. If we can list all supported gates, there won't be the need of decomposing the rij gates (including QFT) theoretically.
So, I need confirmation for removing test_folding_circuit_conversion_error_qiskit. After this, I can commit the latest code.

[andreamari]

LGTM, very clean!
But this is a non-trivial change, so I would wait for an official review by someone from the Mitiq team.
Meanwhile, thank you very much, @gluonhiggs!

[natestemen]

Nice changes, and tests @gluonhiggs! What do you think about adding a warning when we fall into the except QasmException block? This way the user knows the circuit they are working with might have been changed by our code.

[natestemen]

Circuit does not contain an RYY gate. Should the test be renamed or an RYY gate added?

[gluonhiggs]

That's a typo. ry should be replaced by ryy.

[natestemen]

Any reason we should remove this test? It does seem to belong in mitiq/interface/mitiq_qiskit/tests/test_conversions_qiskit.py, but I think it's still useful, right? If the error is not raised anymore (as I think is indicated in #2585 (comment)), then can we change the test to assert the conversion to something happens?

[gluonhiggs]

This test was originally useful for ensuring the detection of unsupported custom gates during conversion. However, in theory, suppose that we can resolve #1073 by this PR, there will be no errors produced in any circuit conversion from qiskit to mitiq. In particular, with the current code (in the PR), we still can convert the circuit without raising any error. That's why I removed the test.
Probably, we could update the test to check if the conversion works correctly for circuits with custom gates, e.g., transpile is called in here (or the except block is executed).

except QasmException:
        # Try to decompose circuit before running
        # This is necessary for converting qiskit circuits with
        # custom packaged gates, e.g., QFT gates
        BASIS_GATES = [
            "sx",
            "sxdg",
            "rx",
            "ry",
            "rz",
            "id",
            "u1",
            "u2",
            "u3",
            "r",
            "x",
            "y",
            "z",
            "h",
            "s",
            "t",
            "cx",
            "cy",
            "cz",
            "ch",
            "swap",
            "cswap",
            "ccx",
            "sdg",
            "tdg",
        ]
        circuit = qiskit.transpile(circuit, basis_gates=BASIS_GATES)
        mitiq_circuit = from_qasm(qasm2.dumps(circuit))

I'd love to hear your thoughts on such an adaptation.

[natestemen]

All that makes sense! My request is that we add one more test, to ensure a circuit with a qiskit.quantum_info.operators.Operator object can be converted correctly. This test verified as such, and we no longer have a test that checks this, if we remove it.

[gluonhiggs]

I have committed a simple test for that. Please have a look!

[natestemen]

LGTM! Just one question, but can be merged once it's checked.

Thank you for your contribution.