Migrate to pyQuil v3 #107

MarquessV · 2022-10-04T23:22:53Z

Here are the changes required to get this plugin on to pyQuil v3! I'm including some of my own targeted questions as comments below.

instruction

MarquessV · 2022-10-12T20:12:27Z

pennylane_forest/qpu.py

@@ -190,7 +163,7 @@ def __init__(
        self.calibrate_readout = calibrate_readout
        self.wiring = {i: q for i, q in enumerate(self.qc.qubits())}

-    def expval(self, observable):
+    def expval(self, observable, shot_range=None, bin_size=None):


When parametric compilation is enabled, the expval for results from all devices (QVM, QPU, WFS) are calculated in the same way. However, when parametric compilation is disabled on the QPU device, we take a different approach to calculating the expval. Why would this differentiation exist, and can we consolidate the implementation across all devices?

Honestly I don't know why are we doing this. It seems like we are creating and executing a new program from scratch, which is not very effective...

@josh146 do you know why we don't use the generated self._samples to compute the expectation value?

Looking at the logic inside the measure_observables function:
https://github.com/PennyLaneAI/pennylane-forest/blob/14709333ed6bb4fa134340f524d89aea44f3cb56/pennylane_forest/qpu.py#L255

It seems it uses a different logic to run the quantum program that the one defined in the generate_samples method of the QVMDevice class:
https://github.com/PennyLaneAI/pennylane-forest/blob/14709333ed6bb4fa134340f524d89aea44f3cb56/pennylane_forest/qvm.py#L230

Maybe we need to override this method in the QPUDevice class?

I took a brief look at doing this while refactoring the devices, but quickly ran into a wall when I realized that the logic in expval is dependent on the observable parameter to generate the right PauliTerm for the Experiment passed to measure_observables.

I would leave this as it is for now. We can create a follow-up issue saying that this should be optimised.

@antalszava

The QPU device allows operator estimation and uses measure_observables from PyQuil under the hood. This feature in the plugin is, however, not compatible with parametric compilation. Therefore, once parametric compilation is turned on, we're falling back to PennyLane's implementation using generate_samples.

A major shortcoming hinted at above too is that when doing operator estimation, we're effectively generating samples using both PennyLane's pipeline and using measure_observables - the first of which should not be done.

See #45 that has just been re-opened.

pennylane_forest/qpu.py

AlbertMitjans

Left some comments on the README.rst file

README.rst

AlbertMitjans

Added more comments on the doc files

doc/devices/qpu.rst

doc/devices/qvm.rst

doc/devices/wavefunction.rst

doc/index.rst

AlbertMitjans

Left some comments in the code. Still need to look at the tests.

pennylane_forest/qpu.py

AlbertMitjans · 2022-10-13T10:50:58Z

pennylane_forest/qpu.py

@@ -190,7 +163,7 @@ def __init__(
        self.calibrate_readout = calibrate_readout
        self.wiring = {i: q for i, q in enumerate(self.qc.qubits())}

-    def expval(self, observable):
+    def expval(self, observable, shot_range=None, bin_size=None):


Honestly I don't know why are we doing this. It seems like we are creating and executing a new program from scratch, which is not very effective...

@josh146 do you know why we don't use the generated self._samples to compute the expectation value?

AlbertMitjans · 2022-10-13T10:56:52Z

pennylane_forest/qpu.py

@@ -190,7 +163,7 @@ def __init__(
        self.calibrate_readout = calibrate_readout
        self.wiring = {i: q for i, q in enumerate(self.qc.qubits())}

-    def expval(self, observable):
+    def expval(self, observable, shot_range=None, bin_size=None):


Looking at the logic inside the measure_observables function:
https://github.com/PennyLaneAI/pennylane-forest/blob/14709333ed6bb4fa134340f524d89aea44f3cb56/pennylane_forest/qpu.py#L255

It seems it uses a different logic to run the quantum program that the one defined in the generate_samples method of the QVMDevice class:
https://github.com/PennyLaneAI/pennylane-forest/blob/14709333ed6bb4fa134340f524d89aea44f3cb56/pennylane_forest/qvm.py#L230

Maybe we need to override this method in the QPUDevice class?

pennylane_forest/qpu.py

pennylane_forest/qvm.py

pennylane_forest/device.py

pennylane_forest/wavefunction.py

pennylane_forest/qpu.py

AlbertMitjans · 2022-10-13T11:19:25Z

The pennylane_forest/device.py file needs reformatting.

Please run black -l 100 pennylane_forest/.

Co-authored-by: Albert Mitjans <a.mitjanscoma@gmail.com>

pennylane_forest/qc.py

MarquessV · 2022-10-18T23:14:17Z

pennylane_forest/qpu.py

@@ -190,7 +163,7 @@ def __init__(
        self.calibrate_readout = calibrate_readout
        self.wiring = {i: q for i, q in enumerate(self.qc.qubits())}

-    def expval(self, observable):
+    def expval(self, observable, shot_range=None, bin_size=None):


I took a brief look at doing this while refactoring the devices, but quickly ran into a wall when I realized that the logic in expval is dependent on the observable parameter to generate the right PauliTerm for the Experiment passed to measure_observables.

AlbertMitjans · 2022-10-19T08:59:33Z

I just realised that the github workflows run the tests using the quilc version 1.22.0.

Probably that is why some of the tests are passing locally but failing here.

I think we should change the following line in .github/workflows/tests.yml from:

      - name: Run Forest Quilc
        run: docker run --rm -d -p 5555:5555 rigetti/quilc:1.22.0 -R

to

      - name: Run Forest Quilc
        run: docker run --rm -d -p 5555:5555 rigetti/quilc -S

This line is duplicated under tests and integration-tests

We should probably change the line in .github/workflows/upload.yml too.

AlbertMitjans · 2022-10-19T09:20:58Z

Regarding the docs, could you try updating the pyquil and pennylane versions in doc/requirements.txt to:

pyquil>=3.0.0,<4.0.0
pennylane>=0.18

AlbertMitjans

Update minimum pennylane version to 0.18 (ISWAP gate was added in the 0.18 release)

setup.py

requirements.txt

pennylane_forest/converter.py

AlbertMitjans

Amazing job with the refactor!

Added some comments.

pennylane_forest/qc.py

pennylane_forest/device.py

pennylane_forest/qc.py

…to-pyquil-v3

pennylane_forest/device.py

AlbertMitjans · 2022-10-20T09:44:30Z

There was a timeout error in the test_qvm.py:test_qubit_unitary test. Should we increase the timeout threshold?

AlbertMitjans

Approved!

MarquessV added 8 commits September 29, 2022 13:39

update pyquil to v3, add qcs-api-client

e0d9fb5

replace ForestConnection with QCSClientConfiguration

7028eea

migrate qvm to pyquil v3

06bde97

reenable forest.qpu and migrate to pyquilv3

ffc9507

migrate forest.(numpy_)wavefunction to pyquil v3

b87450f

fix how timeouts are handled in qvm

d353074

first pass at documentation updates

f3bae8b

forest -> QCS

7d15428

MarquessV closed this Oct 4, 2022

MarquessV added 6 commits October 6, 2022 14:07

get qvm tests passing

da1ae9d

get QPU tests passing

0d9e0d2

relax pyquil requirement, doc suggestions

522ab14

make shot_range, bin_size optional to match QubitDevice, cleanup RESET

46c2ac8

instruction

let pyQuil set its own defaults add missing asserts to tests

70390e9

create separate doc for server config and include it where needed

08c66bf

MarquessV reopened this Oct 12, 2022

MarquessV changed the title ~~Draft: Migrate to pyquil v3~~ Migrate to pyquil v3 Oct 12, 2022

MarquessV changed the title ~~Migrate to pyquil v3~~ Draft: Migrate to pyQuil v3 Oct 12, 2022

josh146 requested a review from AlbertMitjans October 12, 2022 20:19

MarquessV added 2 commits October 12, 2022 13:57

reformat with black

bb4edd4

remove RequestException as it is no longer used in pyQuil

788b3cc

MarquessV commented Oct 12, 2022

View reviewed changes

AlbertMitjans reviewed Oct 13, 2022

View reviewed changes

README.rst Outdated Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

README.rst Outdated Show resolved Hide resolved

AlbertMitjans reviewed Oct 13, 2022

View reviewed changes

pennylane_forest/qpu.py Outdated Show resolved Hide resolved

MarquessV and others added 3 commits October 13, 2022 09:15

Update README.rst

67e65fc

Co-authored-by: Albert Mitjans <a.mitjanscoma@gmail.com>

Update README.rst

22ea520

Co-authored-by: Albert Mitjans <a.mitjanscoma@gmail.com>

Update README.rst

7c4a100

Co-authored-by: Albert Mitjans <a.mitjanscoma@gmail.com>

device refactor: cleanup a few pylint nits

f8c9e68

MarquessV commented Oct 18, 2022

View reviewed changes

AlbertMitjans reviewed Oct 19, 2022

View reviewed changes

setup.py Outdated Show resolved Hide resolved

requirements.txt Outdated Show resolved Hide resolved

AlbertMitjans reviewed Oct 19, 2022

View reviewed changes

pennylane_forest/converter.py Show resolved Hide resolved

AlbertMitjans reviewed Oct 19, 2022

View reviewed changes

pennylane_forest/converter.py Show resolved Hide resolved

AlbertMitjans reviewed Oct 19, 2022

View reviewed changes

AlbertMitjans and others added 13 commits October 19, 2022 14:45

Fix tests

59fd5a3

Merge remote-tracking branch 'upstream/fix-pyqvm-tests' into migrate-…

3b89a18

…to-pyquil-v3

set min pennylane version to 0.18

bae8350

replace plugin operations with pennylane gates

b16f93d

fix pyquil.simulation import

0d6b4a4

remove extra reset in QCDevice, add requested docstrings

61dd4a6

remove trailing newline, simplify simulation import

ac0863a

simplify generate_samples and move pyqvm specific logic to QVMDevice

1ee610b

update docs requirements

ae036e9

bump quilc version in workflows

e78e874

fix typo in method name

d510dd8

make sure program isn't compiled if it's been cached

619dea4

remove extraneous failing test that unnaturally overwrites private value

ef30631

AlbertMitjans reviewed Oct 20, 2022

View reviewed changes

pennylane_forest/device.py Outdated Show resolved Hide resolved

MarquessV added 2 commits October 20, 2022 11:38

pylint: initialize all instance params in __init__

acca6d7

increase EXECUTION_TIMEOUT

9ea5832

AlbertMitjans marked this pull request as ready for review October 21, 2022 07:01

MarquessV changed the title ~~Draft: Migrate to pyQuil v3~~ Migrate to pyQuil v3 Oct 21, 2022

AlbertMitjans approved these changes Oct 24, 2022

View reviewed changes

AlbertMitjans merged commit dcc8eeb into PennyLaneAI:master Oct 24, 2022

AlbertMitjans mentioned this pull request Oct 26, 2022

QPUDevice: Skip generate_samples to avoid executing a circuit twice #108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to pyQuil v3 #107

Migrate to pyQuil v3 #107

MarquessV commented Oct 4, 2022 •

edited

Loading

MarquessV Oct 12, 2022

AlbertMitjans Oct 13, 2022

AlbertMitjans Oct 13, 2022

MarquessV Oct 18, 2022

AlbertMitjans Oct 20, 2022

AlbertMitjans Oct 21, 2022

antalszava Oct 21, 2022 •

edited

Loading

AlbertMitjans left a comment

AlbertMitjans left a comment

AlbertMitjans left a comment

AlbertMitjans Oct 13, 2022

AlbertMitjans Oct 13, 2022

AlbertMitjans commented Oct 13, 2022

MarquessV Oct 18, 2022

AlbertMitjans commented Oct 19, 2022 •

edited

Loading

AlbertMitjans commented Oct 19, 2022 •

edited

Loading

AlbertMitjans left a comment

AlbertMitjans left a comment •

edited

Loading

AlbertMitjans commented Oct 20, 2022

AlbertMitjans left a comment

Migrate to pyQuil v3 #107

Migrate to pyQuil v3 #107

Conversation

MarquessV commented Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava Oct 21, 2022 • edited Loading

Choose a reason for hiding this comment

AlbertMitjans left a comment

Choose a reason for hiding this comment

AlbertMitjans left a comment

Choose a reason for hiding this comment

AlbertMitjans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlbertMitjans commented Oct 13, 2022

Choose a reason for hiding this comment

AlbertMitjans commented Oct 19, 2022 • edited Loading

AlbertMitjans commented Oct 19, 2022 • edited Loading

AlbertMitjans left a comment

Choose a reason for hiding this comment

AlbertMitjans left a comment • edited Loading

Choose a reason for hiding this comment

AlbertMitjans commented Oct 20, 2022

AlbertMitjans left a comment

Choose a reason for hiding this comment

MarquessV commented Oct 4, 2022 •

edited

Loading

antalszava Oct 21, 2022 •

edited

Loading

AlbertMitjans commented Oct 19, 2022 •

edited

Loading

AlbertMitjans commented Oct 19, 2022 •

edited

Loading

AlbertMitjans left a comment •

edited

Loading