-
Notifications
You must be signed in to change notification settings - Fork 121
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
[feature] AutoQASM: Free parameters #762
Merged
laurencap
merged 10 commits into
amazon-braket:feature/autoqasm
from
laurencap:free-parameters
Oct 30, 2023
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
2a8bfca
feature: Add support for FreeParameters
laurencap 047451c
Add new tests and fix broken tests
laurencap 3896cc4
Add more tests
laurencap 1dd5754
Scope down to only floats
laurencap a77de15
Fix subroutine argument processing
laurencap b7c11e2
Scope out conditions
laurencap d65f91b
Fix lint
laurencap e90fd53
Replace type | type with Union
laurencap 0f1f1a2
Fix coverage, remove dup test
laurencap c4ae77a
Add test for gate calibrations
laurencap 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
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
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.
I wonder if there is a good way to define an alias for this union type. Seems like user-defined gates should also technically be using this as a type hint, and we wouldn't want users to have to copy this. Does it make sense to have an
AngleIdentifierType
that we expose asaq.Angle
(just likeaq.Qubit
forQubitIdentifierType
)? Or is that too specific - are there other types that should be supported with this pattern too? Gates can only take angles, but in generalinput
parameters to a program can be other types.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.
It's a good question. Aaron pointed out that we'll want to migrate from float to OpenQASM angles, because gates are not supposed to take floats.
It actually makes the code less clean to include the FreeParameterExpression -- there's a type check somewhere that gate parameters should only be floats, so we have to do extra work to ignore the
FreeParameterExpression
.I feel like allowing the type hint to just hint at what the concrete value should eventually be is probably best in user code. The ParameterExpressions bit feels noisy and unproductive, even if it's true.
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.
Ah I'm noticing now that
gate_calibrations
produce signatures withangle
, so we should prioritize consistency usingangles
everywhere. I didn't realize they are already used in some placesThere 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.
This is just a consideration for the IR we generate. Users will still certainly provide floats
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, gate definitions in OQ3 must use
angle
params, so we do use that for@aq.gate
and@aq.gate_calibration
. OQ3 allows implicitly castingfloat
toangle
, so technically everything else could usefloat
and be fine, I think. But I also agree that if possible we should try to avoid exposing that to users, who just want to usefloat
everywhere.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.
Ah okay, wasn't aware of the implicit cast. That's good news