Fixing definition method for PyGate and PyInstruction

[alexanderivrii]

Summary

This commit fixes a problem when calling definition for a PyGate and PyInstruction. Update: for UnitaryGate we have decided to returning None is the best option.

Details and comments

I have stumbled on this problem while working on porting HighLevelSynthesis to Rust, with the definition method for a custom gate incorrectly returning None (instead of the expected CircuitData), e.g., on the following circuit:

qc_inner = QuantumCircuit(3)
qc_inner.cx(0, 2)
qc_inner.h(1)

qc = QuantumCircuit(5)
qc.x(0)
qc.append(qc_inner.to_gate(), [1, 4, 2])

Looking at the code for PyGate

match self.gate.getattr(py, intern!(py, "definition")) {
    Ok(definition) => {
        let res: Option<PyObject> = definition.call0(py).ok()?.extract(py).ok();
        ...

the problem is that circuit.definition [in the Python space] is a QuantumCircuit and not a callable, with definition.call0(...) returning precisely this error message.

The only question is how can I add a test for this fix? I don't see how I can add something to the Rust space and I don't see how I can add it to the Python space.

[qiskit-bot]

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

• @Qiskit/terra-core

[coveralls]

Pull Request Test Coverage Report for Build 13430994521

Details

• 0 of 10 (0.0%) changed or added relevant lines in 1 file are covered.
• 8 unchanged lines in 3 files lost coverage.
• Overall coverage increased (+0.01%) to 88.116%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/circuit/src/operations.rs	0	10	0.0%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	2	94.29%
crates/circuit/src/operations.rs	2	88.7%
crates/qasm2/src/lex.rs	4	92.73%

Totals
Change from base Build 13421430619:	0.01%
Covered Lines:	78361
Relevant Lines:	88929

---

💛 - Coveralls

[Cryoris]

Going via QuantumCircuitData seems like it would work (though I would ask whether the case of definition being None still returns None with this new solution?) but this will populate a lot fields we don't return in the end (since we only return .data but e.g. .qregs and many others are being built).

Couldn't we fix the previous code by removing the .call0 and directly trying to cast the definition into a CircuitData? This way we'd only build the data we actually return from the function.

[alexanderivrii]

Thanks @Cryoris: the fix looks even better after applying your suggestion to avoid constructing the full QuantumCircuitData. It should indeed return None when the definition is None.

Unfortunately, creating a test from within Rust space seems impossible before the problems mentioned in #13776 are solved. The only thing I managed to do is creating a regular function in operations.rs that does the checking, exposing it to Python, and calling it from one of the python tests, but this solution is horrendously ugly [so not included in this fix].

[mtreinish]

Do we need a definition for UnitaryGate at all? I purposely didn't define it here because the python space one is to call unitary synthesis. In rust it's a bit of a layer violation to do that since it lives in the accelerate crate. While going through python avoids that issue, I'm wondering if it's really necessary because it's not a definition in the strict sense like it is for a concrete gate.

[alexanderivrii]

I need this to handle certain corner-cases arising when porting HighLevelSynthesis to Rust. Even if HighLevelSynthesis runs right after UnitarySynthesis, there may be gates whose plugins / definitions include UnitaryGates, which then need to be recursively synthesized (this is for instance the case for Isometry gates). The current HighLevelSynthesis code in Python handles this by extracting definition for these unitary gates, and proceeding recursively, so it made sense for the Rust code to do so as well. I would be happy to hear alternative approaches.

[mtreinish]

Why not just call the two qubit decomposer on the matrix if you encounter a unitary gate in hls during recursion? Realistically that's what you are doing here, just via a very inefficient path that goes back and forth through python.

[alexanderivrii]

To make sure I understand this correctly, for 1- or 2- qubit unitaries we can use the decomposers already available in rust, and so we can completely avoid going through python; but for larger unitaries the _define method of UnitaryGate uses qs_decomposition, which is only available in python, and so we would still have to go through the python space.

[mtreinish]

Sure, we will have to rely on qsd in python for 3 or more qubit UnitaryGate instances. But that's easy to both encapsulate to only use python when we need it and also substitute the python usage for a rust function if/when we have an implementation of qsd in rust. You can just do something like:

if matches!(inst.view(), OperationRef::Unitary(gate) {
    match gate.num_qubits() {
        0 => panic!(),
        1 => call_euler_decomposer(),
        2 => call_two_qubit_basis_decomposer_with_cx(),
        _ => py_call_qsd(),
    }
}

in hls.

[alexanderivrii]

This makes sense. So I should revert the change to handle unitary gates? Alternatively, I could close this PR, and only keep the HLS PR, since it's the only consumer for the definition method.

[mtreinish]

I think having a standalone PR to fix definition is good. Just revert the unitary gate piece and I think we can merge this.

[alexanderivrii]

Reverted in 5079541.

[mtreinish]

LGTM thanks for fixing this.