fix: cast node.size to IntegerLiteral for qubit register size #258

EuGig · 2024-05-31T18:14:34Z

Issue: #240

Description of changes:
Fixes bug [#240] by casting node.size into IntegerLiteral.
Building a circuit from a quasi string raises an AttributeError exception in case the qubit register is specified as a string literal.

Testing done:
A unit-test is added that checks that the AttributeError is not raised anymore.

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 · 2024-06-10T16:14:05Z

@EuGig Thank you for your contribution! Could you please add details to the PR description on what you have tested, as well as a unit test showing that the issue is resolved by your change?

codecov · 2024-06-10T16:14:06Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a4d7f98) to head (eff72cd).
Report is 1 commits behind head on feature/integer-literal-cast.

Additional details and impacted files

@@                      Coverage Diff                       @@
##           feature/integer-literal-cast      #258   +/-   ##
==============================================================
  Coverage                        100.00%   100.00%           
==============================================================
  Files                                48        48           
  Lines                              3712      3713    +1     
  Branches                            889       891    +2     
==============================================================
+ Hits                               3712      3713    +1

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

EuGig · 2024-06-11T10:23:24Z

Hi @rmshaffer! Tests added 👍
Please, let me now if the PR is ok!

rmshaffer

Thank you for this contribution! I think it would be good to expand the tests a little bit so that the behavior itself is validated, in addition to just seeing that no exception is thrown.

rmshaffer · 2024-06-13T19:22:50Z

test/unit_tests/braket/default_simulator/openqasm/test_interpreter.py

+    try:
+        Interpreter().build_circuit(source=qasm)
+    except AttributeError as exc:
+        pytest.fail("Interpreter raised an AttributeError")


In addition to testing that no exception is thrown (which is good!), could you test that the behavior actually matches what you expect for each of these cases? Especially to see that the qubit register r1 is declared with the size you expect. An example of this is in test_measurement function in this file, where the qubit register is measured into a bit register of the appropriate size, and the measured_qubits property is then asserted to match your expectation.

For example, the first one could be modified to be like this (since the bitstring "10" = 2 as an int):

bit[2] b; qubit["10"] r1; b = measure r1;

And then validate by doing something like:

circuit = Interpreter().build_circuit(qasm) assert circuit.measured_qubits == expected

where expected is set to [0, 1] for this case.

I have some other (slightly wacky) programs in a comment here; maybe these could be added as tests? (they just need testing that there's no uncaught exception - as the QASM itself is invalid according to the spec for most of them)

rmshaffer · 2024-06-13T19:28:14Z

@EuGig to ensure that code formatting is correct, please run tox -e linters locally, commit any changes it makes, and/or fix any problems it finds! thank you.

DanBlackwell · 2024-06-13T20:17:45Z

This is great, thanks @EuGig!

I'm just looking what the spec says about casting here: https://openqasm.com/language/types.html#casting-specifics. I feel like the spec itself could do with some clearing up with regard to what constants are allowed for the 'size' when declaring a register - https://openqasm.com/language/types.html#classical-bits-and-registers does not mention it. Though https://openqasm.com/language/types.html#qubits mentions that indexing by constant values must be supported.

I personally feel as though qubit["1" != "0"] r1; where a bool is cast to int feels a little off, and maybe shouldn't be allowed; but then again int array['a']; is a valid array definition (with length 97) in C, so maybe it's not unacceptably weird.

rmshaffer

Two very minor comments, and then I think this would be ready to merge. Thank you for your patience!

test/unit_tests/braket/default_simulator/openqasm/test_interpreter.py

…ibute 'value'. Fixes bug [amazon-braket#240] by casting node.size into IntegerLiteral

rmshaffer

Leaving a few additional comments, which you can address if you have time. Either way, we'll get this merged by the deadline of Wed 6/19 so that the unitaryHACK bounty can be assigned to you. Thank you for your persistence here!

rmshaffer · 2024-06-17T16:37:41Z

src/braket/default_simulator/openqasm/_helpers/functions.py

+            x.values[len(y.values) :] + [BooleanLiteral(False) for _ in range(len(y.values))]
+            if isinstance(y, ArrayLiteral)
+            else x.values[y.value :] + [BooleanLiteral(False) for _ in range(y.value)]


[minor] this inline logic is getting hard to read - could it be factored out into an internal helper function? perhaps casting.py would be a good place.

rmshaffer · 2024-06-17T16:37:48Z

src/braket/default_simulator/openqasm/_helpers/functions.py

+            [BooleanLiteral(False) for _ in range(len(y.values))]
+            + x.values[: len(x.values) - len(y.values)]
+            if isinstance(y, ArrayLiteral)
+            else [BooleanLiteral(False) for _ in range(y.value)]
+            + x.values[: len(x.values) - y.value]


[minor] this inline logic is getting hard to read - could it be factored out into an internal helper function? perhaps casting.py would be a good place.

ArrayLiteral: { # returns array getattr(BinaryOperator, "&"): lambda x, y: ArrayLiteral( [BooleanLiteral(xv.value and yv.value) for xv, yv in zip(x.values, y.values)] ),

Does this mean that these operators can be applied to array literals too now? It might be better to convert bitstrings to an int type asap where these operations can be handled by python. This would also avoid the possiblity of {0, 3, 6} & 1 (if indeed this does apply to all array literals and not just bitstring), which to me doesn't make much sense.

Also on this theme, it seems as though this is treating bitstring literals in the same way as 0b integer literals ("Integer literals can be written in decimal without a prefix, or as a hex, octal, or binary number, as denoted by a leading 0x/0X, 0o, or 0b/0B prefix. "). I feel as though the 'spirit' of the spec is that the bit string is syntactic sugar for an array containing only 0s and 1s, rather than a number; but the writing does not specify this.

rmshaffer · 2024-06-17T16:39:07Z

test/unit_tests/braket/default_simulator/openqasm/test_interpreter.py

+            """
+                bit[1] b;
+                qubit[1] r1;
+                h r1["0" << "1"];
+                b = measure r1;
+            """,


[minor] these tests which run gates don't validate that they are run correctly, and they also only test h r1[0] and not any other qubit indices. Could they be expanded a bit - for example, to test running gates on additional qubits beyond 0?

I believe this would be (a bit) better as 0 << X is always 0; using 1 << 1 gives 2:

""" bit[3] b; qubit[3] r1; h r1["1" << "1"]; b = measure r1; """,

I don't have a quantum background, but by me eye this should result in the probability state of r1[2] = 0.5 and the other qubits are default.

EuGig · 2024-06-18T12:55:18Z

Thank you @rmshaffer! I'll continue working on the PR.

rmshaffer · 2024-06-18T16:52:32Z

Thank you @rmshaffer! I'll continue working on the PR.

Thank you @EuGig for your work on this so far! Because the unitaryHACK period is ending, I'm going to merge the PR in its current state into the branch feature/integer-literal-cast, and I'll assign the issue to you and close it so that you get credit for your contribution.

I have opened PR #263 to merge that feature/integer-literal-cast branch to main, which we can use to continue working on this until it's ready to go.

If you are able to continue to make some of the suggested changes, please open another PR from your fork which targets the feature/integer-literal-cast branch.

EuGig requested a review from a team as a code owner May 31, 2024 18:14

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

AttributeError: 'ArrayLiteral' object has no attribute 'value'. #240

Closed

rmshaffer requested changes Jun 13, 2024

View reviewed changes

rmshaffer mentioned this pull request Jun 13, 2024

AttributeError: 'ArrayLiteral' object has no attribute 'value'. #240

Closed

rmshaffer changed the title ~~[#240] AttributeError: 'ArrayLiteral' object has no attribute 'value'.~~ fix: cast node.size to IntegerLiteral for qubit register size Jun 13, 2024

EuGig force-pushed the main branch 2 times, most recently from ffd586b to b8abef9 Compare June 14, 2024 15:07

rmshaffer requested changes Jun 14, 2024

View reviewed changes

test/unit_tests/braket/default_simulator/openqasm/test_interpreter.py Outdated Show resolved Hide resolved

test/unit_tests/braket/default_simulator/openqasm/test_interpreter.py Outdated Show resolved Hide resolved

EuGig force-pushed the main branch 2 times, most recently from 1342c83 to f7def25 Compare June 14, 2024 15:33

[amazon-braket#240] AttributeError: 'ArrayLiteral' object has no attr…

eff72cd

…ibute 'value'. Fixes bug [amazon-braket#240] by casting node.size into IntegerLiteral

EuGig force-pushed the main branch from f7def25 to eff72cd Compare June 14, 2024 15:36

rmshaffer approved these changes Jun 14, 2024

View reviewed changes

rmshaffer approved these changes Jun 17, 2024

View reviewed changes

rmshaffer changed the base branch from main to feature/integer-literal-cast June 17, 2024 21:27

rmshaffer merged commit 5ce65b7 into amazon-braket:feature/integer-literal-cast Jun 18, 2024

rmshaffer mentioned this pull request Jun 18, 2024

fix: cast node.size to IntegerLiteral for qubit register size #263

Draft

8 tasks

fix: cast node.size to IntegerLiteral for qubit register size #258

fix: cast node.size to IntegerLiteral for qubit register size #258

Uh oh!

Conversation

EuGig commented May 31, 2024 • edited by rmshaffer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge Checklist

General

Tests

Uh oh!

rmshaffer commented Jun 10, 2024

Uh oh!

codecov bot commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

EuGig commented Jun 11, 2024

Uh oh!

rmshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmshaffer commented Jun 13, 2024

Uh oh!

DanBlackwell commented Jun 13, 2024

Uh oh!

rmshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rmshaffer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EuGig commented Jun 18, 2024

Uh oh!

rmshaffer commented Jun 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

EuGig commented May 31, 2024 •

edited by rmshaffer

Loading

codecov bot commented Jun 10, 2024 •

edited

Loading

rmshaffer commented Jun 18, 2024 •

edited

Loading