-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow float/angle interaction in gate-parameter contexts #35
Conversation
This allows technically invalid OpenQASM 3, but is in practice what Qiskit's OpenQASM 3 exporter outputs due to problems in how it represents symbolic expressions. It's probably better for users to allow the technically invalid OpenQASM 3 than to punish them for things that are actually Qiskit's fault.
This is the same as the multiplication/division part. While it's much less likely for these to appear from the Qiskit export, it makes sense to allow them in non-strict mode for symmetry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise this LGTM, it's a pretty straightforward relaxation of the type rules for the arithmetic operations in non-strict mode. I just had one #nit in the release note otherwise I think it's good to merge.
@@ -233,6 +241,13 @@ def visit_BinaryExpression(self, node: ast.BinaryExpression): | |||
elif isinstance(rhs_type, types.Angle): | |||
const = lhs_type.const and rhs_type.const | |||
out_type = types.Uint(const, None) | |||
# We allow `angle / float` in non-strict mode, because the Qiskit OQ3 exporter does | |||
# not / cannot handle `ParameterExpression` well on output, and often outputs things | |||
# like that. That's invalid OQ3, but it's better to support Qiskit's dodgy output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue tracking this in the language specification? Or was this already discussed and decided that this should not be allowed. It seems odd to me though if this was the decision since it seems pretty natural to perform arithmetic operations between an angle and a float.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An angle
is an integer-like representation as fractions of float
doesn't interact with angle
. That said, there's still awkwardnesses in the whole model around the angle
type being naturally defined on a ring.
This allows technically invalid OpenQASM 3, but is in practice what Qiskit's OpenQASM 3 exporter outputs due to problems in how it represents symbolic expressions. It's probably better for users to allow the technically invalid OpenQASM 3 than to punish them for things that are actually Qiskit's fault.
Fix #25.