search

Qiskit / qiskit-qasm3-import

Importer from OpenQASM 3 to Qiskit's QuantumCircuit
https://qiskit.github.io/qiskit-qasm3-import
Apache License 2.0
15 stars 7 forks source link

Fix implicit integer to float promotions in binary operators #7

Closed jakelishman closed 1 year ago

jakelishman commented 1 year ago

This permits expressions such as 3 * pi / 4; the OpenQASM 3 specification defers to the C promotion rules, which allow this. Previously they were incorrectly forbidden by this converter.

Fix #6.

This addition to the type resolver is still just a huge hack around the AST we're building against not having any "implicit cast" nodes in it, and I'm still hoping to entirely replace this package in the near future, but this is a stop-gap measure to fix a bug reported against Terra.

jlapeyre commented 1 year ago

I approved this. But I have a comment that might go in this PR although it wasn't the point of this PR. Currently, unary minus of unsigned ints is not supported: https://github.com/Qiskit/qiskit-qasm3-import/blob/246e999573c0d7a2162b779d9525b8b5b79c3cf0/src/qiskit_qasm3_import/expression.py#L152-L157

But if we are mostly following C semantics, then we should support it. But how should Qiskit should represent unsigned integers ? You could store the value in an np.uintxx.

Furthermore , Line 152 above is the only one in expression.py where types.Int appears but is not followed by types.UInt. Otherwise, the pair appears 11 times. I think it makes sense to somehow define this pair once. Maybe explicitly as a list or tuple. Or introduce a hierarchy and make Int and UInt subclasses of Integer or something.

jakelishman commented 1 year ago

From offline discussions: let's just leave the unary negation of uint undefined for now; we don't have a direct use of it, and we're intending to replace this package.