Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Stretch] Support
Duration
type in classical expressions. #13844[Stretch] Support
Duration
type in classical expressions. #13844Changes from 87 commits
af4ba52
e423bf9
0a5917b
a97434d
1afc965
86655f1
aaeae9b
a2a444b
8ac2dc3
9f8313c
db9d9cb
71b7e7a
2091557
7307be9
9b30284
ce1faf1
4fee48f
88ab046
c5a230f
7c88d88
c58a7b8
d9e9a8c
4a56150
23b5961
8bf2e4f
111eb32
a839d51
a19b39a
7f02e56
55af327
86b7af9
9b3c821
69eab91
971cc26
25508bf
2c8ce43
ccf9441
c6eab02
edd7806
8afa92e
4e0f2df
15ba943
81c5833
50c31d3
7e43322
2449ee6
e55e189
1d51022
eb8f150
415f62e
d38f3e9
fd66c1d
e9b6d97
52da3cc
a3be688
07771f1
9332ed3
17241c6
ca91bfd
ad87e78
d3bca5f
50deb93
f1dc1a1
1f439a0
166d7b2
10b2c8d
9d6cf39
8021e00
a15141b
ca2785d
e902009
16475c3
6b5930d
be73180
25f1693
ecc343b
b32e6f0
864506d
6bc728a
2d38178
42258b8
481700a
e09762f
cbd55db
f12afb4
8d059cd
e05d9ff
0f14cca
7db8222
34e615b
bbe67a6
0acc450
7fea216
78b2951
97bbeba
49f2af8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Given what we talked about offline, how plausible is it to instead have
stretch
be represented by a new node in theExpr
tree calledConst
?I would like to keep the type-system association we'd newly made with the move of the
const
system toExpr
whereVar
is associated with a mutable l-value,Value
was an immediate value, and nowConst
is some compile-time resolvable object. I know it might involve relatively major changes throughout the control-flow builders, but the DAG wires I think should automatically work, because we weren't adding them anyway.I can live with it if is too much work -
Var(const=True)
andVar(const=False)
is still distinguishable, if not as pretty as having it encoded in the types, not just the values.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.
Honestly, I'm not sure that a
Const
node would even be a good fit for uninitializedstretch
variables. I think we will want aConst
node eventually to represent const definitions with initializers, and I'd expect such a node to be constructed viaexpr.Const.new(name: str, type: types.Type, initializer: expr.Expr)
whereinitializer
gets validated to be a constant expression coercable totype
.I think that
Var
is not a good candidate for representing initialized constants, even with a potentialconst=True
constructor argument, since all initialized constants require an initializer. PerhapsVar
will take an initializer in Qiskit eventually, but it'd be optional and not required to be a constant expression.That said,
Var
can be declared uninitialized, which IMO makes it the more natural fit for representing uninitializedstretch
variables (betweenVar
and an imaginedConst
).What we may want eventually is a third expression kind,
StretchVar
. I didn't want to attempt to introduce that this late in the development. I think we can get away with piggybacking off ofVar
for now as an implementation detail. In #13852 I was planning to makeQuantumCircuit.add_stretch
take only a name (no var) and return just anexpr.Expr
, and then add separate methods for iterating and gettingstretch
variables + filtering them out of the normal var methods. It's unclear to me if they should still show up in the same set of captures, though.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.
I'm not dead certain that this is right. The introduction of the classes calls
Duration
"a length of time, possibly negative", andStretch
"some not-yet-known non-negative duration". By those terms, let's associateDuration
withint
andStretch
withuint
.By comparison to integers: no
int
is ever less than anyuint
(because nouint
can ever represent all the values of anint
), butint
can be greater thanuint
if its width is enough.I think what we might actually be after here is to make
Duration
the "duration expression" type, and makeStretch
an explicit subtype ofDuration
that is non-negative. Then the ordering should beStretch < Duration
, becauseStretch
is a strict subtype of it. So that's backwards to what we have here, but that's expected -Stretch
should be stricter thanDuration
.(Note that I don't mean that
Stretch
inherits fromDuration
in the Python-space class system - OO inheritance is not a good model for type systems - just that the type-theory typeStretch
is a subtype ofDuration
.)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.
Starting out with this comment, since I think it's the most fundamental for us to make sure we get right!
I want to be clear that this is not something I've come up with, but rather something I pulled directly from the OpenQASM docs here:
and later
My assumption based on that last bit is that this must be some kind of compile-time check, separate from QASM's type system, e.g. "evaluate all compile-time expressions, and if a stretch resolves to a negative duration which gets fed into a timing-aware instruction, blow up."
In the second link, we also see this example:
Particularly,
stretch d = a + 2 * c;
implies thatDuration
+Stretch
=>Stretch
, which certainly makes it seem likeStretch
is greater thanDuration
in the typing.Stepping back from that, I think we have 3 separate things going on in all of this:
1000ns
), since there are transformations only applicable to these, likeDuration / Duration => Float
. From what I understandStretch / Stretch
is not defined in QASM.On point 2., my assumption has been that
stretch s = 1000dt;
is valid and implies a stretch-i-ness of 0, hence the ordering relationship in this PR. If that's not the case, then I thinkStretch
andDuration
are unordered. We can't put aStretch
into aDuration
, since we'll lose point 3.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.
Oh yeah, I wasn't thinking you were making it up - I just think it's not quite the cleanest way of representing the OQ3 spec. I see where you're coming from, I just think there's a combination of typing restrictions and value restrictions in play here, and we're not in the right one.
For your points one and two: the statement
stretch d (= Expr)?;
introduces a new degree of freedom into the system. If the initialisation expression is present, it's just a lower bound on the constrained value, clipped to 0 (because stretches can never be negative). That doesn't necessarily mean that the type ofExpr
needs to be<= stretch
- it's actually kind of the other way round; as long as you can embedstretch
in the valid total ordering of the type of the initialisation expression, you're fine. The statementstretch d = a + 2*c;
might be mathematically clearer asstretch d >= a + 2*c;
. So for:I think that's a mistaken inference:
a + 2*c
need not be positive at all, andstretch d = -500dt;
is still a valid initialiser and doesn't require-500dt
to be a valid value instretch
. (In my example, there's two immediate restrictions ond
:d >= -500dt
from the explicit restriction, andd >= 0dt
from the typing.I think this isn't right:
stretch s
introduces a new degree of freedom, andstretch s = 1000dt;
is sayings >= 1000dt
. Otherwise youra + 2*c
also wouldn't have introduced a new degree of freedom - it'd be constrained entirely bya
andc
.Just below your linked section, there's the line
So in the statement
delay[<expr>]
,<expr>
must be a validduration
. There's then a value restriction on that (as opposed to a typing one) that it must resolve (after stretch resolution and constant folding, etc) to a concrete non-negative value. That's not unprecedented in type systems: Python indexes withint
, but negative values have a totally different meaning (you could consider them a different operation, also defined in terms of non-negative integers); and array indexing has an upper bound on the valid values too to make it a valid instruction, regardless of the type width.I would say that a
stretch
always resolves to a valid non-negativeduration
. That astretch
always resolves to aduration
at all is already a statement thatstretch <= duration
in the typing, really.duration
in OQ3 is more properly a "time delta", which can also be negative. Adelay
takes a delta as argument, but is value-constrained to be a positive delta.For point 3: I get what you're saying, but I don't think you necessarily need this to be in the type system, so it's a trade-off between this property or the partial ordering in terms of representable values. There's already plenty of operations that are ill-defined for certain values of a type -
/
of course isn't defined for 0 values on the RHS for integers. I think it's fine to admit all of:a / b
is valid forduration
stretch < duration
a / b
is undefined ifb
resolves to have degrees of freedom.I'd accept the argument that
stretch
andduration
could be unordered because the division operation isn't defined, but if we do that, then we have to special-case all the "cast" logic, which most naturally wants to use the partial ordering of representable values.Using the type system to avoid unresolvable operations would be nice, but it's not universally possible, and I'd call division just one of these cases.
Final bits / clarifying:
box { delay [a] $0; delay [a + 10dt] $1; }
and there's no valid resolution ofa
. I'd argue we just put "division" of stretches into the same vein, and not all divisions are necessarily unresolvable: ifstretch a = 10dt;
, thena / a
is mathematically well-defined to be 1 (becausea
can't be zero - that's the only thing that would be a problem).uint[16]
usefully defined as being< int[32]
for the purposes of safe casting, butint[32]
supports only a subset of the operations ofuint[16]
(no bitwise stuff).stretch
isn't a "type" at all, it's just an unknownduration
(though I probably wouldn't - it's harder to track them, I think).stretch
are the exact names declaredstretch <id>
. I'd call the initialiser expression (if present) aduration
, and interpret it as a limitduration(<stretch id>) >= <duration expr>
.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.
From offline conversations: we actually decided that
stretch
needn't be a separate type fromduration
at all. Most of the rest of this conversation stands, though.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.
Yeah, and thanks for this comment, it was quite helpful.
This is what I wasn't getting—the
=
operator in QASM is more or less overloaded for a stretch declaration. I'd assumed we weren't introducing a new degree of freedom at all. As you pointed out, really thestretch
declaration always introduces a new degree of freedom, and optionally adds a lower bound if there's an "initializer".All of this should be more or less handled now in the later PRs of this series.