feature: support integer division #29

abidart · 2024-06-11T01:26:41Z

Issue #, if available:
#9 and builds on the work of @daredevil3435

Description of changes:

Add a converter for integer division (floor division)
Add an operator that handles floor division
Add a call to the converter in transpiler.py
Add __floodiv__ and __rfloordiv__ methods to IntVar and FloatVar classes for lining purposes

Testing done:
tox -e unit-tests + added unit tests.

Merge Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.

General

I have read the CONTRIBUTING doc
I used the commit message format described in CONTRIBUTING
I have updated any necessary documentation, including READMEs and API docs (if appropriate)

Tests

I have added tests that prove my fix is effective or that my feature works (if appropriate)
I have checked that my tests are not configured for a specific region or account (if appropriate)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rmshaffer

Thanks for making these changes! A few small comments here. Also note you can run tox -e linters to run all the linters locally.

src/autoqasm/operators/arithmetic.py

src/autoqasm/types/types.py

codecov · 2024-06-11T14:19:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (24e6fa1) to head (f44b48e).
Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #29   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           44        46    +2     
  Lines         1991      2043   +52     
  Branches       337       342    +5     
=========================================
+ Hits          1991      2043   +52

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Ryan Shaffer <[email protected]>

rmshaffer

Thanks, I think this is getting very close! A few more small comments.

src/autoqasm/operators/arithmetic.py

test/unit_tests/autoqasm/test_operators.py

rmshaffer · 2024-06-11T16:52:16Z

test/unit_tests/autoqasm/test_operators.py

+float[64] __float_3__;
+__float_3__ = __float_2__ / b;
+c = __float_3__;"""
+        assert main.build().to_ir() == expected_ir


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

def _test_parametric_on_local_sim(program: aq.Program, inputs: dict[str, float]) -> np.ndarray:

device = LocalSimulator(backend=McmSimulator())

task = device.run(program, shots=100, inputs=inputs)

assert isinstance(task, LocalQuantumTask)

assert isinstance(task.result().measurements, dict)

return task.result().measurements

You don't have inputs, so you could ignore that part. And you'd just grab task.result().measurements which is a dict containing the values of all of the variables in the program.

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,

@aq.main def floor_div(): a = aq.IntVar(2) b = 4.3 c = b // a # noqa: F841

results in the warning UserWarning: Integer overflow for value 2.15 and size 32.

This is not directly related to this PR since:

@aq.main def int_to_float(): a = aq.IntVar(2.15)

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?

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.

Amazing, will do!

rmshaffer

🥳 Looks great to me. Thank you, @abidart and @daredevil3435!

daredevil3435 · 2024-06-12T01:45:45Z

🥳 Looks great to me. Thank you, @abidart and @daredevil3435!

Yes. My very first bug bounty!! Thanks you so much for this @rmshaffer 😁

rmshaffer · 2024-06-12T02:47:18Z

@daredevil3435 and for your first bounty, this was not a simple one at all! Thanks for sticking with it 👍

daredevil3435 · 2024-06-12T03:20:53Z

@daredevil3435 and for your first bounty, this was not a simple one at all! Thanks for sticking with it 👍

But you made it look really simple!

jcjaskula-aws

LGTM with some cosmetic comments.

src/autoqasm/operators/arithmetic.py

jcjaskula-aws · 2024-06-12T15:54:53Z

src/autoqasm/types/types.py

@@ -152,10 +152,22 @@ def __init__(self, *args, annotations: str | Iterable[str] | None = None, **kwar
        )
        self.name = program.get_program_conversion_context().next_var_name(oqpy.FloatVar)

+    def __floordiv__(self, other):
+        raise NotImplementedError("Integer division is supported by OpenQASM.")


Not getting the part. If the floor division is supported, why raising a NotImplementedError. Also, I would have expected coverage to pick this up but I don't see any test catching this exception.

@rmshaffer @jcjaskula-aws thank you very much for the feedback! I have a follow-up question if you don't mind. In the PR, the floor division is not "directly" implemented, but rather intercepted by the transpiler and replaced by other operations. I originally added these class methods to trick linters into accepting the integer division involving IntVars/FloatVars, and figured it would make sense to raise an Exception if a user tried to actually call the methods.

Would you suggest I raise a different type of exception, maybe one related to AutoQASM not supporting integer division, or rather remove these methods altogether?

Thanks for the explanation, that's helping me a lot to understand.

There are certainly some efforts to make around autoQASM and linters. Right now, I'd expect we have the same problem at others places too (like for instance with comparison operators). I would just remain consistent with what we have now (meaning removing __floor_div__) and deal with linting overall in another PR.

Super clear! I just committed the changes.

test/unit_tests/autoqasm/test_operators.py

jcjaskula-aws

LGTM, thanks for your contribution.

abidart added 2 commits June 11, 2024 03:17

feature: support integer division

e73e7d1

fix: fix typos

b7a4996

abidart requested a review from a team as a code owner June 11, 2024 01:26

abidart mentioned this pull request Jun 11, 2024

Support integer division with // operator #9

Closed

fix: fix imports

be876b1

rmshaffer requested changes Jun 11, 2024

View reviewed changes

src/autoqasm/operators/arithmetic.py Outdated Show resolved Hide resolved

src/autoqasm/operators/arithmetic.py Outdated Show resolved Hide resolved

src/autoqasm/operators/arithmetic.py Outdated Show resolved Hide resolved

src/autoqasm/types/types.py Outdated Show resolved Hide resolved

abidart and others added 3 commits June 11, 2024 16:42

fix: apply suggestions from code review

6a29f18

Co-authored-by: Ryan Shaffer <[email protected]>

fix: add linting and coverage fixes

e7a50b0

fix: return float in integer division

602b660

rmshaffer requested changes Jun 11, 2024

View reviewed changes

fix: return float in integer division when appropriate

f7ae401

abidart requested a review from rmshaffer June 11, 2024 19:44

rmshaffer previously approved these changes Jun 11, 2024

View reviewed changes

jcjaskula-aws reviewed Jun 12, 2024

View reviewed changes

fix: add cosmetic changes

0cea7ec

abidart dismissed rmshaffer’s stale review via 0cea7ec June 12, 2024 16:54

abidart requested review from jcjaskula-aws and rmshaffer June 12, 2024 16:54

abidart mentioned this pull request Jun 12, 2024

feature: add support for typecasting #27

Merged

5 tasks

fix: remove methods added for linting

f44b48e

jcjaskula-aws approved these changes Jun 12, 2024

View reviewed changes

rmshaffer approved these changes Jun 12, 2024

View reviewed changes

rmshaffer linked an issue Jun 12, 2024 that may be closed by this pull request

Support integer division with // operator #9

Closed

rmshaffer merged commit 360cab1 into amazon-braket:main Jun 12, 2024
15 checks passed

rmshaffer mentioned this pull request Jun 12, 2024

feature: Support integer division with // operator #26

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: support integer division #29

feature: support integer division #29

abidart commented Jun 11, 2024 •

edited

Loading

rmshaffer left a comment

codecov bot commented Jun 11, 2024 •

edited

Loading

rmshaffer left a comment

rmshaffer Jun 11, 2024 •

edited

Loading

abidart Jun 11, 2024 •

edited

Loading

rmshaffer Jun 11, 2024

abidart Jun 11, 2024

rmshaffer left a comment

daredevil3435 commented Jun 12, 2024

rmshaffer commented Jun 12, 2024

daredevil3435 commented Jun 12, 2024

jcjaskula-aws left a comment

jcjaskula-aws Jun 12, 2024 •

edited

Loading

abidart Jun 12, 2024 •

edited

Loading

jcjaskula-aws Jun 12, 2024

abidart Jun 12, 2024

jcjaskula-aws left a comment

	def _test_parametric_on_local_sim(program: aq.Program, inputs: dict[str, float]) -> np.ndarray:
	device = LocalSimulator(backend=McmSimulator())
	task = device.run(program, shots=100, inputs=inputs)
	assert isinstance(task, LocalQuantumTask)
	assert isinstance(task.result().measurements, dict)
	return task.result().measurements

feature: support integer division #29

feature: support integer division #29

Conversation

abidart commented Jun 11, 2024 • edited Loading

Merge Checklist

General

Tests

rmshaffer left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 11, 2024 • edited Loading

Codecov Report

rmshaffer left a comment

Choose a reason for hiding this comment

rmshaffer Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

abidart Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

rmshaffer Jun 11, 2024

Choose a reason for hiding this comment

abidart Jun 11, 2024

Choose a reason for hiding this comment

rmshaffer left a comment

Choose a reason for hiding this comment

daredevil3435 commented Jun 12, 2024

rmshaffer commented Jun 12, 2024

daredevil3435 commented Jun 12, 2024

jcjaskula-aws left a comment

Choose a reason for hiding this comment

jcjaskula-aws Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

abidart Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

jcjaskula-aws Jun 12, 2024

Choose a reason for hiding this comment

abidart Jun 12, 2024

Choose a reason for hiding this comment

jcjaskula-aws left a comment

Choose a reason for hiding this comment

abidart commented Jun 11, 2024 •

edited

Loading

codecov bot commented Jun 11, 2024 •

edited

Loading

rmshaffer Jun 11, 2024 •

edited

Loading

abidart Jun 11, 2024 •

edited

Loading

jcjaskula-aws Jun 12, 2024 •

edited

Loading

abidart Jun 12, 2024 •

edited

Loading