Truncating floats in QASM dump alters circuit's functionality

[burgholzer]

Information

• Qiskit Terra version: 0.18.3 (dates back to 0.14)
• Python version: Any
• Operating system: Any

What is the current behavior?

In #4276, the pi_check function was introduced for pretty printing the list of parameters when dumping a QuantumCircuit to qasm, see
https://github.com/Qiskit/qiskit-terra/blob/637acc0c408d8199dd58780cc3dafabc14e61a0f/qiskit/circuit/instruction.py#L450-L463
This also introduced a fixed truncation (ndigits=8) of floating point numbers that are not close to any fraction or fraction of pi. However, 8 digits is not enough to uniquely identify a double precision floating point number. You need up to 17 significant digits for that. This leads to the following situation:

Let's say I have a circuit that contains, e.g., a phase gate with more than 8 significant digits in its rotation angle.
Then, I dump the circuit to a .qasm file and read that file back into a QuantumCircuit.
If I compare both unitary functionalities (i.e., the respective unitary matrices), these will no longer be equal due to the numerical error introduced by the truncation.

Steps to reproduce the problem

from qiskit import QuantumCircuit, execute, Aer

qc = QuantumCircuit(1)
qc.p(1/4096, 0)

qasm_str = qc.qasm()

backend = Aer.get_backend('aer_simulator')

qc.save_unitary()
job = execute(qc, backend)
result = job.result()
unitary = result.get_unitary(qc)

qc_qasm = QuantumCircuit.from_qasm_str(qasm_str)
qc_qasm.save_unitary()
job = execute(qc_qasm, backend)
result = job.result()
unitary_qasm = result.get_unitary(qc_qasm)
    
print(unitary == unitary_qasm)

yields

[[ True  True]
 [ True False]]

What is the expected behavior?

Merely dumping a file to .qasm should not alter a circuit's functionality.

Suggested solutions

A quick fix for this would be to just change the default value for the ndigits parameter in the respective function call to 17.

[jakelishman]

This is very true, and I'm not a fan of pi_check in general for anything other than pretty visualisations. In part, I think things were done like this because serialisation to and from OpenQASM 2 was never intended to be completely lossless (various Terra operations simply can't be represented in OpenQASM 2 already), and at the time, hardware probably wasn't capable of realising the difference in pulses at that level of precision. It's not an ideal choice now, for sure.

We can look into raising the number of digits here. Due to some unfortunate coupling with other parts of the code, I think it possibly has a couple of knock-on changes too. Our new OpenQASM 3 exporter is a bit more separated out from the Terra internals, which will hopefully mean things like this will be far more controllable once that arrives (see #6565, due to land in Terra 0.19).

I will say that "dumping a file to .qasm should not alter a circuit's functionality" is slightly mistaken, though - OpenQASM 2 is not a serialisation format for Terra circuits, it's just an output format we support. If you need complete serialisation, try qiskit.circuit.qpy_serialization.{dump,load} - this is a complete binary format.

[burgholzer]

I see your point that OpenQASM 2 is not intended as a lossless serialisation option. However, in this case, where this is not an inherent limitation of the OpenQASM 2 specification, I believe it would be beneficial to fix this.
I would have expected this to be a rather local issue in the Instruction.qasm() routine (i.e., just changing the magic number there), but then again I am not that familiar with the particular details.

Great to see, that OpenQASM 3 export is coming. Looking forward to that!

[enavarro51]

pi_check serves a nice purpose in finding fractions of pi, whether for visualization or qasm output. The ndigits is just there to tell pi_check what to do with the float if no fraction of pi is found. In visualizations, this is set to something that looks good in the display. For qasm, it really should be up to the user how many float decimals are included in the output, since I doubt everyone needs 17, so I'd recommend adding a kwarg to Instruction.qasm, to let the user choose.

Of course, it doesn't solve the problem that OpenQASM 2 and QuantumCircuit are not one-to-one, but it would give users more flexibility.

[levbishop]

I tend to agree that having lossless-as-possible mode for qasm export is a useful enhancement. Both for the reason of interfacing with external tools, and also for debug purposes.

[jakelishman]

I'm on board with adding more precision to QASM 2 output, I'm just not certain the best way to go about it here. We can't (officially) add anything to the signature of Instruction.qasm, because that's a public interface that we expect people to inherit from, so starting to pass it extra keywords would break compatibility.

A couple of ways we could go:

• break compatibility anyway, and just wrap calls to Instruction.qasm in a try: ...; except TypeError.
• replace the ndigits in all Qiskit Instruction.qasm definitions with a reference to a global variable, context-managed by QuantumCircuit.qasm, which does accept the keyword.
• change the output to always use more digits in every case

None of those are super nice to me, but there may be other options. The last is probably the nicest, but it doesn't solve the other underlying problem that calls to pi_check are hard-coded, so anything that falls close to a multiple of pi will get (very) lossily converted - we'd also have to change eps, but I think that's an absolute tolerance, whereas what we want here is really a ULP tolerance.

This functionality will (or should) be available in the OpenQASM 3 exporter. The way internal instructions provide their QASM definition (at the moment) is by returning AST nodes, so that's a lossless conversion, and the printer is completely separated, and can take many more options - one of the ones already implemented is to completely disable all the pi_check conversions (which, to me at least, might want to be the default). It should be not too hard to ensure that the default printing behaviour for floats is "enough digits to uniquely identify a single float with the given precision".

[burgholzer]

The second option certainly provides the most flexibility. QuantumCircuit.qasm could then just accept ndigits and eps keywords. Although, I am not so sure whether this flexibility is really needed.
One possibility could be to add a numeric flag/keyword to the QuantumCircuit.qasm function that just skips the pi_check call all together and just emits an as precise as possible floating point representation (i.e., up to 17 decimals) for every parameter.
Or the other way around: always print the numerically-accurate representation (skipping pi_check) and provide a flag/keyword for pretty printing/exporting (possibly at the cost of precision).

Having pi_check disabled per default sounds like a good idea for OpenQASM 3 export. Then, users can decide whether they want to pretty print/export their circuit at the cost of precision.

[jakelishman]

The problem is that QuantumCircuit.qasm internally calls Instruction.qasm on all its data elements, which may be overridden by user classes as it's a public API that we allow/encourage subclassing. We can't pass down any flags to our classes and avoid raising TypeError in ones users have already written without a bunch of hackery (e.g. try/except to see if they support the parameter, or having our classes access global state under our control). These issues are almost all about maintaining proper backwards compatibility in public APIs; if we were making these changes in private, internal interactions only, yeah, I agree, it's not a hard problem. We can't have Terra 0.19 break valid code users have written for Terra 0.18 - we have to have at least a three-month deprecation period.

[burgholzer]

I get that. But then I would suggest the easiest workaround/fix that does not need any kind of deprecation would just be to change the magic ndigits constant in the Instruction.qasm() from 8 to 17.
In addition to that it might make sense to explicitly specify a smaller eps (something in the range of 1e-12 to 1e-14), in order to lessen the impact of the approximation to fractions of pi.

In this fashion, no compatibility is broken and the export to OpenQASM 2 gets a lot more accurate / less lossy.

[enavarro51]

@jakelishman If @burgholzer's short-term solution is acceptable, I can take care of it.

[EliasLF]

I just ran into this bug/peculiarity of Instruction.qasm(), too. Any updates on @burgholzer's proposed short-term solution?
An eps value of 1e-12 in pi_check seems to work fine in my case.

[jakelishman]

Sorry this fell through the cracks. I'm ok with accepting a PR that changes eps to be tighter here, if that helps people.

[burgholzer]

No worries. Would you also be willing to change ndigits from 8 to 17 in order to get double precision instead of float precision?