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.

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

Add rewrite_arithmetic handler #1210

Merged
merged 2 commits into from
May 5, 2020

Conversation

braised-babbage
Copy link
Contributor

@braised-babbage braised-babbage commented May 4, 2020

Description

When compiling native quil programs, the first step is to do "arithmetic rewriting", which turns compound gate parameters into simple memory references. The motivation for this is that the underlying hardware can do a certain amount of arithmetic, but not everything (multiplication and division are done via arithmetic shifts, and so are restricted to powers of two, also real arithmetic is done with fixed point numbers which results in a loss of precision, and so on).

This arithmetic rewriting is actually quite simple, but presently requires a call to the quilc client. This is a bit annoying, as native_quil_to_executable does not otherwise need quilc, and in my personal experience this dependency can be a bit burdensome when developing/testing native_quil_to_executable functionality. This PR deprecates this call with a simple and explicit rewrite_arithmetic function.

In the long term we will need to reconsider entirely this approach (if we are to allow for run-time updates to parameter values), but for now this is how things are.

Checklist

  • The above description motivates these changes.
  • There is a unit test that covers these changes.
  • All new and existing tests pass locally and on Travis CI.
  • Parameters and return values have type hints with PEP 484 syntax.
  • Functions and classes have useful Sphinx-style docstrings.
  • All code follows Black style and obeys flake8 conventions.
  • The changelog is updated, including author and PR number (@username, gh-xxx).

@braised-babbage braised-babbage requested a review from a team as a code owner May 4, 2020 19:01
@braised-babbage braised-babbage force-pushed the feature/rewrite-arithmetic-natively branch from 7cbaf9d to 94662cf Compare May 4, 2020 19:03
@braised-babbage braised-babbage force-pushed the feature/rewrite-arithmetic-natively branch from bb13f39 to cd4a332 Compare May 4, 2020 19:23
@@ -8,6 +8,9 @@ Changelog

### Improvements and Changes

- Added a PyQuil only `rewrite_arithmetic` handler, deprecating the previous
Copy link
Contributor

Choose a reason for hiding this comment

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

don't h8, hyphenate

pyquil-only

Comment on lines 402 to 411
assert (
response.quil
== Program(
"DECLARE __P2 REAL[2]",
"DECLARE theta REAL[1]",
"DECLARE beta REAL[1]",
"RZ(__P2[0]) 0",
"RZ(__P2[1]) 0",
).out()
)
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i don't make the rules!

Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

Looks good! On a longer horizon (for the general SDK) we may want this to live in libquil, but for the time being it's good to reduce complexity by replacing this call to quilc.

@@ -343,3 +350,62 @@ def test_local_conjugate_request(benchmarker):
def test_apply_clifford_to_pauli(benchmarker):
response = benchmarker.apply_clifford_to_pauli(Program("H 0"), PauliTerm("I", 0, 0.34))
assert response == PauliTerm("I", 0, 0.34)


def test_rewrite_arithmetic_no_params():
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think these could live in api/test_rewrite_arithmetic.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call.

@braised-babbage braised-babbage merged commit e5c06ff into master May 5, 2020
@braised-babbage braised-babbage deleted the feature/rewrite-arithmetic-natively branch May 6, 2020 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants