feature: add support for typecasting #27

abidart · 2024-06-07T16:05:48Z

Issue #, if available:
Related to #10.

Description of changes:
In an attempt to add support for int and float conversion, I made 3 changes:

a typecast converter that converts int and float calls
a typecast operator that adds a different behavior to int and float calls when they act on a qasm type
a line in the transpiler file to incorporate the typecast transformer

The goal is to add a functionality such that the following two compile to the same QASM instructions, or very similar:

syndrome = int(measure([0,1]))
################################
a1 = aq.IntVar(measure(1))
a0 = aq.IntVar(measure(0))
syndrome = 2*a1 + a0

Testing done:

I know that the current code fails when the call to int or float is nested, for example syndrome = 2 * int(measure([0,1])). I need to fix that.
The current changes pass tox -e 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 starting this! I've left a few comments/suggestions to help debug. Of course you'll want to add unit tests once the code is working, but that can be saved for a later iteration.

By the way, in case it's useful you can enable verbose autograph output via environment variable, e.g. you can do this in a Jupyter notebook cell:
%env AUTOGRAPH_VERBOSITY 10
This will print out the intermediate transpiled program when you call build() on an AutoQASM program - I find this very useful when working on converters.

src/autoqasm/transpiler/transpiler.py

src/autoqasm/operators/typecast.py

src/autoqasm/converters/typecast.py

abidart · 2024-06-07T18:59:42Z

I added 3 unit tests but I was not sure if they should go into test_api.py, test_operator.py, or test_converter.py, what do you think? Also, do you think that the expected_irs look reasonable?

rmshaffer

This looks like it's on the right track! A few more comments/suggestions here.

test/unit_tests/autoqasm/test_api.py

rmshaffer · 2024-06-07T19:11:45Z

test/unit_tests/autoqasm/test_api.py

+        expected_ir = """OPENQASM 3.0;
+qubit[2] __qubits__;
+bit[2] __bit_0__ = "00";
+__bit_0__[0] = measure __qubits__[0];
+__bit_0__[1] = measure __qubits__[1];"""


This doesn't look right. I would expect the output IR to contain something like:

int[32] __int_1__ = __bit_0__; int[32] test = 2 * __int_1__;

For debugging, might be interesting to know what the IR output is if you modify the program to be like this:

@aq.main(num_qubits=2) def main(): test = int(2 * measure([0, 1]))

or like this:

@aq.main(num_qubits=2) def main(): test = 2 * int(measure([0, 1])) return test

I checked and using

@aq.main(num_qubits=2) def main(): test = 2 * IntVar(measure([0, 1]))

results in the same IR. I think the discrepancy came from the fact that the variable test was not being used after declared. Your second suggestion helped me realize that adding return test (or other lines that make use of test) results in an IR with the int[32] lines. I updated the unit test to reflect this.

On a second note, I did not add support for arithmetic operations on BitVara in this PR because I focused on int typecasting. To add the arithmetic operations, would you suggest I add methods like __mul__ and __rmul__ to the BitVar class (where self gets casted to an IntVar) or use a converter to intercept the arithmetic operations and cast the BitVars to IntVars?

Oh, good question. I think it's ok to leave out the arithmetic operations from this PR. I think it's ok (and maybe desirable) that the user must manually cast the BitVar to an int before doing arithmetic on it.

src/autoqasm/operators/typecast.py

test/unit_tests/autoqasm/test_api.py

codecov · 2024-06-10T14:04:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f83bb15) to head (f50afec).
Report is 8 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##              main       #27   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           47        49    +2     
  Lines         2057      2090   +33     
  Branches       343       345    +2     
=========================================
+ Hits          2057      2090   +33

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

test/unit_tests/autoqasm/test_api.py

src/autoqasm/operators/typecast.py

rmshaffer · 2024-06-10T14:09:16Z

test/unit_tests/autoqasm/test_api.py

+        @aq.main(num_qubits=2)
+        def main():
+            test = 2 * int(measure([0, 1]))  # noqa: F841
+            return test


It'd be worth having two versions of this test, one without the return and one with the return. In both cases the test variable should be declared and assigned - it shouldn't matter whether it is used afterward.

Added! Could you confirm that in the case where there is no return statement, there should not be an instruction involving the 2 *. Currently, I am getting:

with return

OPENQASM 3.0; output int[32] test; qubit[2] __qubits__; bit[2] __bit_0__ = "00"; __bit_0__[0] = measure __qubits__[0]; __bit_0__[1] = measure __qubits__[1]; int[32] __int_1__; __int_1__ = __bit_0__; test = 2 * __int_1__;

without return

OPENQASM 3.0; qubit[2] __qubits__; bit[2] __bit_0__ = "00"; __bit_0__[0] = measure __qubits__[0]; __bit_0__[1] = measure __qubits__[1]; int[32] __int_1__; __int_1__ = __bit_0__;

That's interesting - I still would expect the test variable to be declared and assigned 2 * __int_1__ there. But functionally it doesn't matter, of course. I expect this would be an issue in the assignment converter and outside the scope of your PR. It shouldn't block moving forward with your changes.

Maybe I can look into this in a new issue/PR? I noticed that

@aq.main(num_qubits=2) def main(): test = 4 * IntVar(2)

results in

OPENQASM 3.0; qubit[2] __qubits__;

but

@aq.main(num_qubits=2) def main(): test = IntVar(2)

leads to

OPENQASM 3.0; qubit[2] __qubits__; int[32] test = 2;

Yes, if you'd like to look into this separately, that would be great! I'd recommend opening a new issue with those details.

rmshaffer · 2024-06-10T14:12:52Z

test/unit_tests/autoqasm/test_api.py

+        expected_ir = """OPENQASM 3.0;
+qubit[2] __qubits__;
+bit[2] __bit_0__ = "00";
+__bit_0__[0] = measure __qubits__[0];
+__bit_0__[1] = measure __qubits__[1];"""


Oh, good question. I think it's ok to leave out the arithmetic operations from this PR. I think it's ok (and maybe desirable) that the user must manually cast the BitVar to an int before doing arithmetic on it.

rmshaffer · 2024-06-10T14:15:20Z

I added 3 unit tests but I was not sure if they should go into test_api.py, test_operator.py, or test_converter.py, what do you think?

@abidart Probably test_operators.py makes the most sense. test_api.py is getting too large and needs to be split apart at some point.

abidart · 2024-06-12T17:03:27Z

@rmshaffer I pushed a new commit with cosmetic changes based on the review from #29 anticipating similar comments.

test/unit_tests/autoqasm/test_operators.py

laurencap · 2024-06-12T21:55:12Z

test/unit_tests/autoqasm/test_operators.py

+def test_int_typecasting_on_string():
+    @aq.main(num_qubits=2)
+    def main():
+        test = int("101", 2)  # noqa: F841


Should we raise an error, instead?

Hello @laurencap, thank you so much for the review! My goal with this test was to have a "normal" call to int within a program to check that the built-in int function gets called—correctly, with all passed arguments—whenever the first input does not involve a QASM type. Could you explain me how can I change it?

I believe this is working as intended, but maybe the confusion could be cleared up by changing the name of the test function to make it more clear what is being tested - e.g. test_int_typecasting_on_python_string_not_captured

feature: add support for typecasting

66d5459

abidart mentioned this pull request Jun 7, 2024

Support arithmetic on measurement results #10

Closed

rmshaffer requested changes Jun 7, 2024

View reviewed changes

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

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

src/autoqasm/converters/typecast.py Outdated Show resolved Hide resolved

abidart added 3 commits June 7, 2024 20:11

fix: changes to int typecasting

8a09bdd

fix: add support for args and kwargs in typecasting)

3f63229

fix: add unit tests for int typecasting

17cd488

abidart marked this pull request as ready for review June 7, 2024 18:59

abidart requested a review from a team as a code owner June 7, 2024 18:59

abidart requested a review from rmshaffer June 7, 2024 19:00

rmshaffer requested changes Jun 7, 2024

View reviewed changes

fix: update typecasting tests

0ca1252

abidart requested a review from rmshaffer June 7, 2024 23:16

fix: remove unused dependency

16fe026

rmshaffer requested changes Jun 10, 2024

View reviewed changes

abidart added 2 commits June 10, 2024 17:02

fix: update operator to force casting in the correct location

c477d76

fix: type hint

d64bfad

abidart requested a review from rmshaffer June 10, 2024 15:36

rmshaffer previously approved these changes Jun 11, 2024

View reviewed changes

fix: add cosmetic changes

638d3dd

abidart dismissed rmshaffer’s stale review via 638d3dd June 12, 2024 17:01

abidart requested a review from rmshaffer June 12, 2024 17:03

Merge branch 'main' into main

129af14

rmshaffer reviewed Jun 12, 2024

View reviewed changes

test/unit_tests/autoqasm/test_operators.py Show resolved Hide resolved

Update test/unit_tests/autoqasm/test_operators.py

a158117

jcjaskula-aws reviewed Jun 12, 2024

View reviewed changes

test/unit_tests/autoqasm/test_operators.py Show resolved Hide resolved

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

Support arithmetic on measurement results #10

Closed

laurencap reviewed Jun 12, 2024

View reviewed changes

fix: add xfail test

f71190d

abidart requested a review from jcjaskula-aws June 13, 2024 10:11

jcjaskula-aws previously approved these changes Jun 13, 2024

View reviewed changes

rmshaffer previously approved these changes Jun 13, 2024

View reviewed changes

rmshaffer and others added 2 commits June 13, 2024 09:59

Merge branch 'main' into main

52f99f1

fix: clarify test name

f50afec

abidart dismissed stale reviews from jcjaskula-aws and rmshaffer via f50afec June 13, 2024 17:31

rmshaffer approved these changes Jun 13, 2024

View reviewed changes

rmshaffer merged commit e2bb431 into amazon-braket:main Jun 13, 2024
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add support for typecasting #27

feature: add support for typecasting #27

abidart commented Jun 7, 2024 •

edited

Loading

rmshaffer left a comment

abidart commented Jun 7, 2024

rmshaffer left a comment

rmshaffer Jun 7, 2024

rmshaffer Jun 7, 2024

abidart Jun 7, 2024 •

edited

Loading

rmshaffer Jun 10, 2024

codecov bot commented Jun 10, 2024 •

edited

Loading

rmshaffer Jun 10, 2024

abidart Jun 10, 2024

rmshaffer Jun 10, 2024

abidart Jun 10, 2024 •

edited

Loading

rmshaffer Jun 11, 2024

rmshaffer Jun 10, 2024

rmshaffer commented Jun 10, 2024

abidart commented Jun 12, 2024

laurencap Jun 12, 2024

abidart Jun 12, 2024

rmshaffer Jun 13, 2024

feature: add support for typecasting #27

feature: add support for typecasting #27

Conversation

abidart commented Jun 7, 2024 • edited Loading

Merge Checklist

General

Tests

rmshaffer left a comment

Choose a reason for hiding this comment

abidart commented Jun 7, 2024

rmshaffer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidart Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 10, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidart Jun 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmshaffer commented Jun 10, 2024

abidart commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abidart commented Jun 7, 2024 •

edited

Loading

abidart Jun 7, 2024 •

edited

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

abidart Jun 10, 2024 •

edited

Loading