-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 const
classical expressions.
#13811
Merged
Merged
Changes from 21 commits
Commits
Show all changes
47 commits
Select commit
Hold shift + click to select a range
af4ba52
WIP
kevinhartman e423bf9
Add try_const to lift.
kevinhartman 0a5917b
Try multiple singletons, new one for const.
kevinhartman a97434d
Revert "Try multiple singletons, new one for const."
kevinhartman 1afc965
Remove Bool singleton test.
kevinhartman 86655f1
Add const handling for stores, fix test bugs.
kevinhartman aaeae9b
Fix formatting.
kevinhartman a2a444b
Remove Duration and Stretch for now.
kevinhartman 8ac2dc3
Cleanup, fix const bug in index.
kevinhartman 9f8313c
Fix ordering issue for types with differing const-ness.
kevinhartman db9d9cb
Fix QPY serialization.
kevinhartman 71b7e7a
Make expr.Lift default to non-const.
kevinhartman 2091557
Revert to old test_expr_constructors.py.
kevinhartman 7307be9
Make binary_logical lift independent again.
kevinhartman 9b30284
Update tests, handle a few edge cases.
kevinhartman ce1faf1
Fix docstring.
kevinhartman 4fee48f
Remove now redundant arg from tests.
kevinhartman 88ab046
Add const testing for ordering.
kevinhartman c5a230f
Add const tests for shifts.
kevinhartman 7c88d88
Add release note.
kevinhartman c58a7b8
Add const store tests.
kevinhartman d9e9a8c
Address lint, minor cleanup.
kevinhartman 25508bf
Reject const vars in add_var and add_input.
kevinhartman 2c8ce43
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman ccf9441
Implement QPY support for const-typed expressions.
kevinhartman c6eab02
Remove invalid test.
kevinhartman edd7806
Update QPY version 14 desc.
kevinhartman 8afa92e
Fix lint.
kevinhartman 4e0f2df
Add serialization testing.
kevinhartman 15ba943
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman 50c31d3
Test pre-v14 QPY rejects const-typed exprs.
kevinhartman 7e43322
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman 07771f1
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman d3bca5f
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman 50deb93
Revert visitors.py.
kevinhartman f1dc1a1
Address review comments.
kevinhartman 1f439a0
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman 166d7b2
Improve type docs.
kevinhartman 10b2c8d
Merge branch 'main' of github.com:Qiskit/qiskit into const-expr
kevinhartman 9d6cf39
Revert QPY, since the old format can support constexprs.
kevinhartman 8021e00
Move const-ness from Type to Expr.
kevinhartman a15141b
Revert QPY testing, no longer needed.
kevinhartman ca2785d
Add explicit validation of const expr.
kevinhartman e902009
Revert stuff I didn't need to touch.
kevinhartman 16475c3
Update release note.
kevinhartman 6b5930d
A few finishing touches.
kevinhartman 8e466a6
Address review comments.
kevinhartman File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kevinhartman marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
--- | ||
features_circuits: | ||
- | | ||
The classical realtime-expressions module :mod:`qiskit.circuit.classical` can now represent | ||
constant types and operations on them. All :class:`~.types.Type` classes now have a bool | ||
:attr:`~.types.Type.const` property which is used to mark const-ness and enforce const | ||
invariants across the types system. | ||
|
||
To create a const value expression use :func:`~.expr.lift`, setting ``try_const=True``:: | ||
|
||
from qiskit.circuit.classical import expr | ||
|
||
expr.lift(5, try_const=True) | ||
# >>> Value(5, Uint(3, const=True)) | ||
|
||
The result type of an operation applied to const types is also const:: | ||
|
||
from qiskit.circuit.classical import expr | ||
|
||
expr.bit_and(expr.lift(5, try_const=True), expr.lift(6, try_const=True)).type.const | ||
# >>> True | ||
kevinhartman marked this conversation as resolved.
Show resolved
Hide resolved
|
Oops, something went wrong.
Oops, something went wrong.
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.
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.
Possibly this is clearer as
since the
const
bit is pretty fundamental here.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 believe this comment is actually no longer valid, since I decided not to "decide" const-typed expressions can never be lvalues. Originally, I'd changed the implementation of
is_lvalue
to returnFalse
for any const-typed expression.The reason for my deferral was that it seemed reasonable to me to use a
store
instruction to initialize a const variable, even though we block it at the moment. I'm not sure if it's valid in QASM to initialize a constant without an initializer, e.g.: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, I don't know if that's valid or not. But tbh, even if it is valid, I'd just rather not support it in Qiskit - a few languages let you forward-declare storage, and initialise it in some scope, as long as the set of initialising statements dominates the set of statements using the variable. Rust is one of those - it's valid to have
let x;
and then assign tox
in some scope, provided the structure of the program guarantees thatx
is assigned if it's used.I'd rather just not support that in Qiskit, just to reduce complexity. If we gain more complex handling for classical control flow analysis later, we can always revisit.
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.
Sounds good to me. I thought of the
let x;
pattern in Rust too, but then I realized that Rust doesn't allowconst x;
, which is closer to the semantics I'm alluding to here—it requires an inline initializer.