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

feat: Add a with_loop method to Program #1717

Merged

Conversation

MarquessV
Copy link
Contributor

@MarquessV MarquessV commented Dec 15, 2023

Description

Closes #1712

Checklist

  • The PR targets the master branch
  • The above description motivates these changes.
  • The change is atomic and can be described by a single commit (your PR will be squashed on merge).
  • All changes to code are covered via unit tests.
  • Parameters and return values have type hints with PEP 484 syntax.
  • Functions and classes have useful Sphinx-style docstrings.
  • (New Feature) The docs have been updated accordingly.

@MarquessV MarquessV linked an issue Dec 15, 2023 that may be closed by this pull request
@rigetti-githubbot
Copy link

rigetti-githubbot commented Dec 15, 2023

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
7102 6228 88% 87% 🟢

New Files

No new covered files...

Modified Files

File Coverage Status
pyquil/quil.py 84% 🟢
TOTAL 84% 🟢

updated for commit: 53d8833 by action🐍

@MarquessV MarquessV marked this pull request as ready for review December 15, 2023 16:42
@MarquessV MarquessV requested a review from a team as a code owner December 15, 2023 16:42
@notmgsk
Copy link
Contributor

notmgsk commented Dec 18, 2023

Haven't looked over it in detail, but one thought I had immediately was: the time seems ripe to get away from the "numshots" nomenclature, and instead use the generic "loop".

Copy link
Contributor

@notmgsk notmgsk left a comment

Choose a reason for hiding this comment

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

Code-wise, looks good.

I don't think I'm a fan of the UX, however. Having to do both p.wrap_in_numshots_loop() and p.apply_numshots_loop() feels awkward/unintuitive to me.

What about calling the new method with_loop(iterations, mem_ref, start_label, end_label)?

docs/source/advanced_usage.rst Outdated Show resolved Hide resolved
docs/source/programs_and_gates.rst Outdated Show resolved Hide resolved
# labels to mark the beginning and end of the loop.
looped_program = p.apply_numshots_loop(shot_count, Label("start-loop"), Label("end-loop"))
print(looped_program.out())
print(looped_program.num_shots)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would also be useful to extend the example to include execution (i.e. that the memory reference has to be provided a value in qc.run or whatever).

Co-authored-by: Mark Skilbeck <mark.skilbeck@rigetti.com>
@MarquessV
Copy link
Contributor Author

@notmgsk I hear you there, that's essentially how the API is implemented for quil-rs/py since it doesn't have the baggage of an existing API.

In the context of pyQuil, wrap_in_numshots_loop() is so commonplace that I worry if we provide a separate with_loop method, users will accidentally doubly nest their programs with a numshots loop and Quil loop. Maybe that is overly paranoid though, happy to change it. Curious what @kalzoo thinks, since he authored the issue.

@kalzoo
Copy link
Contributor

kalzoo commented Jan 16, 2024

Code-wise, looks good.

I don't think I'm a fan of the UX, however. Having to do both p.wrap_in_numshots_loop() and p.apply_numshots_loop() feels awkward/unintuitive to me.

What about calling the new method with_loop(iterations, mem_ref, start_label, end_label)?

I like this. It's a good point - so long as wrap_in_numshots_loop remains supported for bw compatibility, it makes sense to think about how we would approach this from scratch rather than assuming all user jobs are wrapped in a shot count and then that loop is applied to the Quil.

Further, letting both methods "start from scratch" rather than working in series does match the reality that there can be two loops: one in metadata as num_shots and one (or more) in the program body.

the time seems ripe to get away from the "numshots" nomenclature, and instead use the generic "loop".

I also agree with this. num_shots does make sense from a physics perspective and we can leave it named as such in program metadata, but what we're really adding here is a fixed-count loop, which may or may not be "shots" of the experiment depending on how it's used and what else is added to the program.

@MarquessV MarquessV changed the title feat: Add a apply_numshots_loop method to Program feat: Add a with_loop method to Program Jan 16, 2024
@MarquessV
Copy link
Contributor Author

@notmgsk @kalzoo - thanks for the feedback, I refactored this PR around the suggested with_loop method.

pyquil/quil.py Outdated
Comment on lines 331 to 354
Return a copy of the ``Program`` wrapped in a Quil loop that will execute ``num_iterations`` times.

This loop is implemented with Quil and should not be confused with the ``num_shots`` property set by
:py:meth:`~pyquil.quil.Program.wrap_in_numshots_loop`. The latter is metadata on the program that
can tell an executor how many times to run the program. In comparison, this method adds Quil
instructions to your program to specify a loop in the Quil program itself.

The loop is constructed by wrapping the body of the program in classical Quil instructions.
The given ``iteration_count_reference`` must refer to an INTEGER memory region. The value at the
reference given will be set to ``num_iterations`` and decremented in the loop. The loop will
terminate when the reference reaches 0. For this reason your program should not itself
modify the value at the reference unless you intend to modify the remaining number of
iterations (i.e. to break the loop).

The given ``start_label`` and ``end_label`` will be used as the entry and exit points for
the loop, respectively. You should provide unique ``JumpTarget``\\s that won't be used elsewhere
in the program.

If ``num_iterations`` is 1, then a copy of the program is returned without any changes. Raises a
``TypeError`` if ``num_iterations`` is negative.

:param loop_count_reference: The memory reference to use as the loop counter.
:param start_label: The ``JumpTarget`` to use at the start of the loop.
:param end_label: The ``JumpTarget`` to use to at the end of the loop.
Copy link
Contributor

Choose a reason for hiding this comment

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

this is great

@MarquessV MarquessV merged commit b398817 into master Jan 27, 2024
22 checks passed
@MarquessV MarquessV deleted the 1712-utility-apply-num-shots-loop-directly-to-program-quil branch January 27, 2024 02:50
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.

Utility: apply num-shots loop directly to program Quil
4 participants