Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: Handling for autoqasm subroutine return values #633

Merged
merged 5 commits into from
Jul 24, 2023
Merged

fix: Handling for autoqasm subroutine return values #633

merged 5 commits into from
Jul 24, 2023

Conversation

rmshaffer
Copy link
Contributor

@rmshaffer rmshaffer commented Jul 21, 2023 •
edited
Loading

Issue #, if available:

Description of changes:

  • Correctly translate autoqasm subroutines which return literal/constant values.
  • Subroutines return the oqpy return variable now instead of the aq.Program instance, allowing them to be handled correctly when returned from other subroutines or using them as part of expressions.

Testing done:

  • Tests added to cover newly-supported cases.
  • All existing tests pass after tweaking order of variable declaration in a handful of places that were affected by this change. (The resulting programs are equivalent; there is no behavior change.)

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

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.

@codecov
Copy link

codecov bot commented Jul 21, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (feature/autoqasm@2a9d5a6). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 58e1d97 differs from pull request most recent head d4a32c5. Consider uploading reports for the commit d4a32c5 to get more accurate results

@@                 Coverage Diff                 @@
##             feature/autoqasm     #633   +/-   ##
===================================================
  Coverage                    ?   99.91%           
===================================================
  Files                       ?      147           
  Lines                       ?     8665           
  Branches                    ?     1866           
===================================================
  Hits                        ?     8658           
  Misses                      ?        4           
  Partials                    ?        3

rmshaffer
rmshaffer commented Jul 21, 2023
View reviewed changes
src/braket/experimental/autoqasm/operators/assignments.py

if isinstance(value, oqpy.base.Var):
# TODO: If name is defined in value, it might be different from target_name.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yitchen-tim I believe this TODO is outdated now. It looks like we handle the target and value names correctly. But please let me know if I am misunderstanding.

@rmshaffer rmshaffer marked this pull request as ready for review July 21, 2023 19:52
@rmshaffer rmshaffer requested a review from a team as a code owner July 21, 2023 19:52
@rmshaffer rmshaffer changed the title fix: Handling for autoqasm functions with unnamed return values fix: Handling for autoqasm subroutine return values Jul 21, 2023
laurencap
laurencap reviewed Jul 21, 2023
View reviewed changes
src/braket/experimental/autoqasm/api.py
Comment on lines +197 to +198
to the conversion process. Or, the oqpy variable returned from the converted function,
if this is a subroutine conversion.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nit, should these be hanging indent? I think we've been inconsistent about it. I guess it's not so important with a return value, cause there aren't other values to delineate from

Copy link
Contributor Author

@rmshaffer rmshaffer Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the linter enforces no hanging indent on return value docstrings - seems consistent with most of the other functions I see. (But unfortunately I don't think the linter checks docstrings on private functions...)

laurencap
laurencap reviewed Jul 21, 2023
View reviewed changes
src/braket/experimental/autoqasm/api.py
@@ -302,11 +308,15 @@ def _convert_program_as_subroutine(
oqpy_program.set(return_variable, subroutine_function_call)
elif return_instance is not None:
return_variable = aq_types.wrap_value(return_instance)
oqpy_program.declare(return_variable)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this modify return_variable?

Copy link
Contributor Author

@rmshaffer rmshaffer Jul 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it only modifies the oqpy_program state.

laurencap
laurencap approved these changes Jul 21, 2023
View reviewed changes
Copy link
Contributor

@laurencap laurencap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple questions for my understanding, and a suggestion to move the tests to test_types. Looks good!

src/braket/experimental/autoqasm/types/py_to_oqpy.py Show resolved Hide resolved
test/unit_tests/braket/experimental/autoqasm/test_api.py Outdated Show resolved Hide resolved
test/unit_tests/braket/experimental/autoqasm/test_api.py Outdated Show resolved Hide resolved
test/unit_tests/braket/experimental/autoqasm/test_api.py Outdated Show resolved Hide resolved
@rmshaffer rmshaffer merged commit 043ce72 into feature/autoqasm Jul 24, 2023
26 checks passed
@rmshaffer rmshaffer deleted the rmshaffer/autoqasm-retval-fixes branch July 24, 2023 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laurencap laurencap laurencap approved these changes

@AbeCoull AbeCoull Awaiting requested review from AbeCoull AbeCoull was automatically assigned from aws/amazon-braket-maintainers

@yitchen-tim yitchen-tim Awaiting requested review from yitchen-tim

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rmshaffer @laurencap