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

Remove requirements used for RAM-style sampling #874

Merged
merged 9 commits into from
Aug 23, 2019
Merged

Remove requirements used for RAM-style sampling #874

merged 9 commits into from
Aug 23, 2019

Conversation

ecpeterson
Copy link
Contributor

This PR removes the requirements enforced in PyQVM that let it always do efficient run_and_measure-style sampling of bitstrings.

On reflection, it might be desirable to offer a separate pathway that does do run_and_measure-style sampling, rather than remove it outright. I'll return to this before making this a real PR.

@ecpeterson ecpeterson requested a review from notmgsk April 5, 2019 18:51
@notmgsk
Copy link
Contributor

notmgsk commented Apr 5, 2019

tests are unhappy

@ecpeterson ecpeterson mentioned this pull request May 14, 2019
Closed
@kylegulshen
Copy link
Contributor

@ecpeterson I had to modify read_memory, and did so by guessing at your intention; I'm concerned that I don't see any use of self._bitstrings despite the fact that you added code to populate it.

I think that the behavior of pyqvm.run is now consistent with qvm.run. I see the appeal of adding run_and_measure but figure we could add this in a separate PR.

@ecpeterson
Copy link
Contributor Author

The self._bitstrings business is because the abstract base class QAM uses it in a computation: https://github.com/rigetti/pyquil/pull/873/files#diff-05dd0b8b019ab1542c364ec236cbbc2aL112 . There's a comment around that same point in this PR that that line becomes unnecessary once #873 gets merged. Maybe we should merge that too / first!

@ecpeterson
Copy link
Contributor Author

I'm going to rebase this onto the current master; I think the parser changes that have happened in the meantime are why pytest is balking.

@ecpeterson ecpeterson mentioned this pull request May 15, 2019
Merged
@joshcombes
Copy link
Contributor

joshcombes commented May 22, 2019 •
edited
Loading

@ecpeterson and @kylegulshen it looks good, and if merged it will help resolve some issues/errors in some of our benchmarking code :)

I see you want to have both options i.e. re-include run_and_measure, and Kyle is suggesting including that option in a future PR. What is the current thinking?

@ecpeterson ecpeterson marked this pull request as ready for review May 23, 2019 18:50
@ecpeterson ecpeterson requested a review from a team as a code owner May 23, 2019 18:50
@ecpeterson
Copy link
Contributor Author

I don't have any preference about the ultimate fate of r_a_m. The guard that this PR removes was put there to make sure the user is in the case where r_a_m doesn't have confusing behavior. Without this guard, there's only the usual muddy waters to put up with.

@joshcombes joshcombes requested review from notmgsk and removed request for notmgsk June 10, 2019 21:38
@joshcombes
Copy link
Contributor

sorry @notmgsk I was trying to re request a review.

appleby
appleby reviewed Jun 18, 2019
View reviewed changes
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

As usual, I am useless for reviewing semantics, but my nitpick game is on point.

pyquil/pyqvm.py Outdated Show resolved Hide resolved
pyquil/pyqvm.py Show resolved Hide resolved
pyquil/pyqvm.py Outdated Show resolved Hide resolved
appleby
appleby approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

My nits have been picked, so I'll approve.

Was the previous "sampled bitstrings" version more efficient but otherwise equivalent for "run and measure" style programs? If so, would it make sense to just check and see if you have "run and measure" style program and use the sampled-bitstrings method if possible, otherwise fallback to this new version? Apologies in advance if I have completely missed the point :-)

Hmm, reviewing the PR conversation thread, it seems like adding "run and measure" style behavior back might happen in a follow-on PR, which sounds reasonable to me.

@ecpeterson
Copy link
Contributor Author

You haven't missed the point, that's definitely a desirable thing to do—people might even be displeased with a slower measurement scheme being the default. (I prefer robust to fast, but what can you do?)

It is my intention to go back and add some 'if we never did anything r_a_m-illegal, then...' logic in a future PR.

@karalekas karalekas added this to the v2.11 milestone Jul 29, 2019
@karalekas karalekas changed the title remove requirements used for RAM-style sampling Remove requirements used for RAM-style sampling Aug 23, 2019
@karalekas karalekas merged commit b0548d5 into master Aug 23, 2019
@karalekas karalekas deleted the remove-RAM-sampling branch August 23, 2019 06:56
appleby added a commit that referenced this pull request Oct 20, 2019
@appleby
Re-instate support for user-defined gates in PyQVM.execute
bd909e9 
In #874, PyQVM was changed to remove the restriction for
run-and-measure style programs. In the process, PyQVM.execute lost the
ability to execute programs with user defined gates, which apparently
broke pennylane-forest's PyQVM support.

This commit re-instates the block of code for extracting defined_gates
from the Program passed to PyQVM.execute, and with it the ability to
execute Programs with (non-parametric) defgates.

Fixes #1059

See also: PennyLaneAI/pennylane-rigetti#22
appleby added a commit that referenced this pull request Oct 20, 2019
@appleby
Reinstate support for user-defined gates in PyQVM.execute
23ce01f 
In #874, PyQVM was changed to remove the restriction for
run-and-measure style programs. In the process, PyQVM.execute lost the
ability to execute programs with user defined gates, which apparently
broke pennylane-forest's PyQVM support.

This commit reinstates the block of code for extracting defined_gates
from the Program passed to PyQVM.execute, and with it the ability to
execute Programs with (non-parametric) defgates.

Fixes #1059

See also: PennyLaneAI/pennylane-rigetti#22
@appleby appleby mentioned this pull request Oct 20, 2019
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@appleby appleby appleby approved these changes

@joshcombes joshcombes joshcombes approved these changes

@karalekas karalekas karalekas approved these changes

@notmgsk notmgsk Awaiting requested review from notmgsk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.11
Development

Successfully merging this pull request may close these issues.
6 participants
@ecpeterson @notmgsk @kylegulshen @joshcombes @appleby @karalekas