OpenQASM returned by convert_to_openqasm includes opaque gates. about pygsti

[sserita]

Hi @hodgestar, thanks for the feedback!

I am however moving this from a bug to a discussion for the following reason: This was an explicit decision based on the deprecation of the id instruction from Qiskit a while back. The deprecation warning states to use the delay instruction instead; unfortunately, delay is not defined in qelib1.inc. The easiest way to solve this is just to define delay as an opaque instruction, which is then parsed into the Delay() operation by Qiskit's QASM2 parser (see https://docs.quantum.ibm.com/api/qiskit/qasm2#legacy-compatibility).

I will admit that it is an unsatisfactory solution, and is due to an inherent mismatch between Qiskit's QuantumCircuit class and OpenQASM 2. I would potentially be in favor of moving to QASM 3, where the proper way to do this is with delays and stretches - my only concern would be maintaining backwards compatibility with pyGSTi's current qasm output, which is used in some places for serialization, e.g. IBMQExperiment objects.

We'd certainly welcome a PR if you are interested in developing a QASM3 transpiler instead. You would need to sign a CLA first - email me at [email protected] for a copy. Otherwise, a QASM3 transpiler will likely be on our to-do list at some point, but likely not for several months.

[hodgestar]

@sserita Thanks for the quick reply. Very happy for this to be a discussion instead of a bug.

For adding OpenQASM 3 support, my thought was to just add a convert_to_openqasm3 method. The convert_to_openqasm method could then be deprecated at some point down the line. Alternatively, one could add a version parameter to convert_to_openqasm. Somehow the separate method seems nicer to me, but I'd be happy to write it either way.

I'm still curious about why these zero delays or id gates are needed here.

[sserita]

Re: why they are needed: That's a fair question. It's almost certainly for some legacy reason - I will check with other team members and see if we remember why these were added and if we can remove them. That way, we could potentially fix this issue for you in the short-term, and spawn off the QASM3 converter as a new feature.

I can see both methods working, but I'm inclined to agree that a separate function seems a little cleaner. That way, we can just use a deprecated decorator on the old function whenever we want to remove it (which may not be quite yet, as there is still a lot of QASM2 floating around).

We would still need a Contributor License Agreement (CLA) from you before we could accept a PR - email me at [email protected] to get that process started.

[hodgestar]

@sserita Any luck finding out why these strange operations are needed?

Once I know the answer I can make an OpenQASM 3 implementation.

[sserita]

Hey @hodgestar, there's now a flag to remove those delay instructions. You can pass include_delay_on_idle=False into convert_to_openqasm and it should drop the delays and be a valid OpenQASM. It's currently just in bugfix-release-0.9.12.1 branch, but that release will be out shortly.

I'm going to close this out if that's acceptable, and we can create a new issue whenever we start working on the OpenQASM 3.0 emitter.

[hodgestar]

Thanks @sserita. I was waiting on hearing what the delays were for before starting an OpenQASM 3 PR, but I'll take the new flag as meaning that the delays for zero time are probably not necessary.

Happy with this issue being closed. I'll open a PR for the OpenQASM 3 variant when I get there.

[sserita]

Yep that sounds good! In OpenQASM 3, we actually have delay as a valid instruction (and stretches to deal with variable time of the other gates so we don't have to hardcode a 0), but I can handle that logic when we get there if that's something that doesn't interest you in implementing. 😅

[hodgestar]

I'll figure it out -- you can comment on the PR if I get it wrong. :)