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
feature: support integer division #29
feature: support integer division #29
Changes from 3 commits
e73e7d1
b7a4996
be876b1
6a29f18
e7a50b0
602b660
f7ae401
0cea7ec
f44b48e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 58 in src/autoqasm/operators/arithmetic.py
Codecov / codecov/patch
src/autoqasm/operators/arithmetic.py#L56-L58
Check warning on line 157 in src/autoqasm/types/types.py
Codecov / codecov/patch
src/autoqasm/types/types.py#L157
Check warning on line 160 in src/autoqasm/types/types.py
Codecov / codecov/patch
src/autoqasm/types/types.py#L160
Check warning on line 171 in src/autoqasm/types/types.py
Codecov / codecov/patch
src/autoqasm/types/types.py#L171
Check warning on line 174 in src/autoqasm/types/types.py
Codecov / codecov/patch
src/autoqasm/types/types.py#L174
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.
For full validation of your code's correctness - you could also run this program on a simulator and validate that the results of the floor division are what you expect.
Running on a simulator is pretty simple, see here for an example:
autoqasm/test/unit_tests/autoqasm/test_parameters.py
Lines 28 to 33 in 24e6fa1
You don't have
inputs
, so you could ignore that part. And you'd just grabtask.result().measurements
which is a dict containing the values of all of the variables in the program.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.
Thank you so much, this helped a ton! Should add tests involving the local simulator to
test_operators.py
as well or was this suggestion mostly for local development?On the local development note, I realized that every time OpenQASM casts a FloatVar that is not a round number to an IntVar I get a warning coming from
openqasm/_helpers/casting.py:96
. For example,results in the warning
UserWarning: Integer overflow for value 2.15 and size 32.
This is not directly related to this PR since:
nets the same warning. The warning can show up when floats/FloatVars are involved in
//
because this PR casts the result of the normal division to an IntVar. Do you think I should do something in the context of this PR to silence the warning when the user does integer division or should I leave it?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 see, that's a warning from the simulator here:
https://github.com/amazon-braket/amazon-braket-default-simulator-python/blob/a4d7f98cb123ae6a1092972e728d2dbb93cb27b5/src/braket/default_simulator/openqasm/_helpers/casting.py#L83-L99
It looks like that warning should be modified to account for that case - not sure yet if that change should happen in the autoqasm repo, or in the amazon-braket-default-simulator-python repo. Feel free to open an issue here to track it though, with the details you just provided! It definitely doesn't need to be fixed within this PR.
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.
Amazing, will do!