[Stretch] Support Float type in classical expressions.
#13832
Conversation
This reverts commit e2b3221.
Types that have some natural order no longer have an ordering when one of them is strictly greater but has an incompatible const-ness (i.e. when the greater type is const but the other type is not).
We need to reject types with const=True in QPY until it supports them. For now, I've also made the Index and shift operator constructors lift their RHS to the same const-ness as the target to make it less likely that existing users of expr run into issues when serializing to older QPY versions.
This is probably a better default in general, since we don't really have much use for const except for timing stuff.
Since we're going for using a Cast node when const-ness differs, this will be fine.
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 13598643113Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
I wasn't going to have this, but since we have DANGEROUS Float => Int, and we have Int => Bool, I think this makes the most sense.
By making const-ness a property of expressions, we don't need any special serialization in QPY. That's because we assume that all `Var` expressions are non-const, and all `Value` expressions are const. And the const-ness of any expression is defined by the const-ness of its operands, e.g. when QPY reconstructs a binary operand, the constructed expression's `const` attribute gets set to `True` if both of the operands are `const`, which ultimately flows bottom-up from the `Var` and `Value` leaf nodes.
There was a problem hiding this comment.
github says I have 12 comments, but I think most of them are either ludicrously minor or just jokes
I think there was only two real ones (though they appear in a couple of places each) - questions about the exception handling and one about maybe making the float width explicit.
| if kind is CastKind.DANGEROUS: | ||
| raise TypeError(f"cannot cast '{expr}' to '{type}' without loss of precision") | ||
| raise TypeError(f"no cast is defined to take '{expr}' to '{type}'") | ||
| return None |
There was a problem hiding this comment.
Checking this branch out locally, I can see several uses of _coerce_lossless in this file that appear to have unchecked return values after this change.
In particular: _equal_like, _binary_relation, and _shift_like (but I might have missed one or two).
I think the _equal_like and _binary_relation ones shake out ok because of type.kind checks higher up, but it would be good to put a one-liner comment into each explaining that.
The _shift_like one has a potential bug:
from qiskit.circuit.classical import expr, types
expr.shift_left(expr.lift(5.0), 3, types.Uint(8))is an AttributeError.
I don't know why I gave shift_left and shift_right a cast field within them - I think I must have been attempting to save some typing with inserting an extra expr.lift on the left operand when passing an integer literal and trying to fix the width (since the left- and right operands don't need to have equal widths in shifts, so there's no inference) - but at any rate, now it's possible to fail the cast in there.
There was a problem hiding this comment.
Good catch. I've fixed this and added a test in 0f14cca
kevinhartman
added
the
mod: circuit
Summary
Adds a new
Floattype to the classical expression system. This is necessary to express timing relationships between durations and stretches in expressions.In a follow-up(s), I'll add
DurationandStretchtypes as well as binary operators for+,-,*and/, and then plumb all of this into the existing circuit variable tracking infra, integrate with theDelayinstruction, support serialization, etc.Details and comments
Currently based on #13811. Here's a readable diff in the meantime.Floattype is considered of an "unspecified width", in accordance with the machine-precisionfloatin OpenQASM3. If we add a fixed-width float (similar to what we've got forUint), then it shouldn't be difficult to add an optional constructor argument toFloatforwidth, defaulted toNone. I didn't opt to do this since I imagine fixed-width floats aren't desperately needed for stretches, and they would've taken too long to stamp out.expr.liftnow supports Python floats.UintorBooltoFloatto be unsafe.Floatcannot be mixed withUintin binary operations. You must cast one of them, first.Floatfor one reason or another (e.g. not losslessly coercable toBool). The arithmetic operations to come (+,-,*and/) will support them.To-do