Hi @BillGatesNephew,

thanks for opening, this is great! The feature sounds good to me, this will be a good medium sized project. I think this is just at the size for an RFC. Do you mind sketching out a design in an RFC, mainly: How are you planning to map a custom gate to Cirq? How about recursively defined gates, are you going to keep the structure somehow? How will that play with Cirq protocols like _decompose_ and _unitary_? Should we use CircuitOperations for this (it's work in progress by @95-martin-orion)?

Feel free to prototype while you are writing the RFC, and share it on your fork, happy to give feedback!

Also, a side-note: the current implementation direction might change in the coming months and be replaced by a qiskit/cirq mapping if we find that that's smaller / cheaper to maintain, there are two reasons:

1. It was driven by the fact that we did not want to introduce qiskit as a dependency, so I basically reimplemented the parser using ply for a subset of the language. We are currently actively working on decoupling modules from cirq core, which will allow for introduction of large dependencies like that. When that happens it might be that relying on qiskit's parser and just providing a simple mapping between qiskit and Cirq gates would be a simpler design than maintaining a whole parser.
2. With OpenQASM 3.0 out, we would like to support it too (Support OpenQASM 3.0 #3518) - but for that again rewriting a parser could be potentially more expensive than the mapping approach.

Having said that - it might be that the parser is actually cheaper to maintain than the mapping, we'll have to evaluate that down the line after the packaging extraction is done.

@balopat Yeah I wouldn't mind working on an RFC for it after I think a little more about it and consider the points you mentioned. Thanks.

This seems to have gone stale. @balopat do you have any updates on the qiskit dependency?

We made progress with extracting our first module, but no progress was made on the qiskit side of things. I still think it's a good idea to revisit the implementation if someone's up for it. In the meantime if someone wants to implement the custom gate definitions in QASM parser, now we have the CircuitOperation class as well, that can help map those custom constructs.

Does this still need an RFC ? I would like to give a shot at this.

Yes, AFAIK nobody is working on it right now.

Before diving too deep you might want to get some agreement from the maintainers regarding whether to use decomposition or subcircuits here. IIUC QASM custom gates are unitary only (https://qiskit.github.io/openqasm/language/gates.html#hierarchically-defined-unitary-gates), so I'd lean toward a decomposition based approach, where as subcircuits map more closely to QASM subroutines (https://qiskit.github.io/openqasm/language/subroutines.html).

But that's just me. @tanujkhattar / @95-martin-orion / @MichaelBroughton do you have any top-level opinions on that? (Also, is bringing in the full qiskit dependency still something anyone is considering in the near term)?

My take on this:

1. The only difference I see between QASM custom gates and subroutines is the return value. CircuitOperations seem reasonable for either case - they don't have explicit "return values", but can expose measurement results.
2. Using decomposition requires dynamically defining new gate types during parsing. This seems inconvenient compared to simply generating CircuitOperations with the required definitions.

Note: while CircuitOperations are fully functional in cirq-core, their serialization for use in cirq-google is still WIP (see #4380).

1. Subroutines are a part of OpenQasm3 specs, but they are not part of openQasm2.0. OpenQasm2 only supports custom gate with gate keyword.
2. On Circuit Operation vs _decompse_, using circuit operation will require extra qubits to be defined for each gate defined in asm ?

2. On Circuit Operation vs _decompse_, using circuit operation will require extra qubits to be defined for each gate defined in asm ?

Extra qubits shouldn't be required for this. A CircuitOperation can use one set of qubits in its definition, and be mapped to a different set of qubits for its use in a circuit (e.g. to emulate applying a QASM gate to some qubits). For example:

q0, q1, q2 = cirq.LineQubit.range(3)

# This operation is defined on {q0, q1}
op = cirq.CircuitOperation(
    cirq.FrozenCircuit(cirq.H(q0), cirq.CNOT(q0, q1))
)

# This circuit only acts on qubits {q1, q2}
circuit = cirq.Circuit(
    op.with_qubits(q1, q2),  # applied to {q1, q2}
    cirq.measure(q1, q2, key='m'),
)

As long as the CircuitOperation's qubits are reassigned, the qubits used in its definition are irrelevant - they are simply the default values, and can overlap with (or be disjoint from) the actual target qubits without issue.

cirq cync note: This is accepted but low priority and will be done with community support.