Port PennyLane-Cirq to QubitDevice #19

johannesjmeyer · 2020-02-03T09:52:56Z

PennyLane introduces the QubitDevice baseclass for plugins in the next release (0.8.0). As QubitDevice eases the implementation drastically, this PR ports the PennyLane-Cirq plugin to this base device class.

As a side effect, PennyLane-Cirq now supports tensor observables.

This PR also adresses some changes in Cirq that resulted in depreceation warnings.

codecov · 2020-02-04T14:30:27Z

Codecov Report

Merging #19 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #19      +/-   ##
==========================================
- Coverage   98.73%   98.67%   -0.06%     
==========================================
  Files           6        6              
  Lines         158      151       -7     
==========================================
- Hits          156      149       -7     
  Misses          2        2

Impacted Files	Coverage Δ
pennylane_cirq/cirq_device.py	`100% <100%> (ø)`	⬆️
pennylane_cirq/simulator_device.py	`98.38% <100%> (-0.08%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3e8865...3c5fe9f. Read the comment docs.

antalszava · 2020-02-04T15:14:01Z

pennylane_cirq/cirq_device.py

-                    # be calculated later
-                    w, U = np.linalg.eigh(Hmat)
-                    self._eigs[Hkey] = {"eigval": w, "eigvec": U}
+        # TODO: get pre rotated state here


Actually I'm a little confused with the handling of pre-rotation state and rotated state. In the implementation of default.qubit, the probability is given for the rotated state but the state is given for the pre_rotated_state. I think this is somewhat inconsistent, isn't it?

Here in Cirq I have the additional problem that I can't really extract the pre_rotated_state without much hassle so that currently, state is the rotated state.

What is your opinion on this, @antalszava ?

And @josh146

This is slightly intentional. Now that there is qml.prob, we no longer want the user to access dev.probability, so for consistency this should always represent the actual device probability.

For state, however, we are still in an awkward position where there is no equivalent to extract it from the qnode yet

So what do you think of me keeping the post-rotation state as dev.state? It is possible to extract the pre-rotation state, but it will not make the code clearer...

I think it is fine for now. dev.state should only ever be used for debugging, and we can attach caveats to it that depend on the device (e.g. a lot of hardware simulators don't even store any state)

I added a docstring that explains this

pennylane_cirq/simulator_device.py

antalszava · 2020-02-04T15:20:03Z

pennylane_cirq/simulator_device.py

-    def pre_measure(self):
-        super().pre_measure()
+    def apply(self, operations, rotations=None, **kwargs):
+        super().apply(operations, rotations, **kwargs)


Suggested change

super().apply(operations, rotations, **kwargs)

super().apply(operations, rotations=rotations, **kwargs)

This would be in case kwargs=={} is True and apply being called only with passing operations to it, since then I think rotations would default to None and there might be an error arising from passing too many positional arguments (None is going to be passed as the second argument). Might be wrong though.

I removed the argument rotations entirely now to be consistent with the signature of apply from the QubitDevice class.

antalszava · 2020-02-04T15:25:39Z

pennylane_cirq/simulator_device.py

-            )
-        else:
-            return self.sample(observable, wires, par).var()
+            return super().generate_samples()


How about the case of analytic==True and returning qml.sample?

I see no problem with that? The logic of deciding when to call generate_samples is implemented in QubitDevice, so I just have to assure that samples are properly generated when it is called. And for analytic mode I can safely piggyback onto the default implementation.

This is the part of the device.execute():

if (not self.analytic) or circuit.is_sampled: self._samples = self.generate_samples()

for self.analytic==True and circuit.is_sampled SimulatorDevice.generate_samples() will be called and since self.analytic==True the if statement evaluates to True on line 158, and QubitDevice.generate_samples() will be called.

Whereas, if self.analytic==False and circuit.is_sampled, SimulatorDevice.generate_samples() still gets called, but the if statement on line 158 evaluates to False and the code block below gets executed.

If this is so (and I didn't confuse myself or mess up something here 😅 ), then how come that there is a different logic for these two cases?

All you say is totally correct. There is a different logic for the analytic==False case because in that case I don't generate the samples myself but I use Cirq's own measurement scheme. I did this because I wanted to use as much of Cirq's inbuilt API as possible

This is the difference between simulator.run (analytic==False) and simulator.simulate (analytic==True)

tests/test_cirq_device.py

antalszava · 2020-02-04T15:30:14Z

tests/test_cirq_device.py

-            ("RX", [2], [cirq.Rx(2)]),
-            ("RX.inv", [1.4], [cirq.Rx(-1.4)]),
-            ("RX.inv", [-1.2], [cirq.Rx(1.2)]),
-            ("RX.inv", [2], [cirq.Rx(-2)]),


Perhaps keeping 1-1 rotations tests (e.g. RX, Rot and their inverses) could be nice

They are still there, it is just misaligned due to the blacking

antalszava

Looks good @johannesjmeyer 😊 Once the comments are addressed, should be merge-ready

Co-Authored-By: Josh Izaac <josh146@gmail.com>

…ennylane-cirq into qubit_device_port

This reverts commit d4a4933.

…ce port has taken place

antalszava · 2020-02-04T20:19:35Z

requirements.txt

@@ -1,3 +1,3 @@
-pennylane>=0.7
+pennylane>=0.8


This was extending the functionality of #21 with QubitDevice being used in this plugin.

johannesjmeyer added 7 commits January 27, 2020 12:37

First step of Cirq port

ef8c1df

Fix test_apply.py

6ba3bb3

Finish test_apply fix

bab630f

Fix more tests

e1a2ba3

Remove pre_measure tests

d224ebd

Start fixing return value tests

50f30ab

Continue test refactor

884146d

johannesjmeyer added the WIP Work in Progress label Feb 3, 2020

johannesjmeyer added 16 commits February 4, 2020 13:46

Fix generate_samples

b5d6018

Fix test_expval

7467f29

Adjust to Cirq 0.7

116508b

Remove superfluous test

2135f0c

Fix apply operation test

1405c58

Continue fix of test_simulator_device.py

09ca2a9

Finish fixing test_simulator_device.py

02144d9

Fix test_var.py

7221e4e

Fix tensor tests

a0bc314

Fix test_expval tensor tests

28b2164

Fix tests

2526bbd

Black

43b12f3

Remove black format tags

481b73c

Black again

3d26c3a

Black main code

3504868

Update capabilities

0504de5

johannesjmeyer and others added 4 commits February 4, 2020 15:30

Merge branch 'master' into qubit_device_port

f3e8865

Linting

153c4d1

Update changelog

3d7407a

Update changelog again

47a9add

johannesjmeyer changed the title ~~[WIP] Port PennyLane-Cirq to QubitDevice~~ Port PennyLane-Cirq to QubitDevice Feb 4, 2020

antalszava reviewed Feb 4, 2020

View reviewed changes

pennylane_cirq/simulator_device.py Show resolved Hide resolved

antalszava reviewed Feb 4, 2020

View reviewed changes

tests/test_cirq_device.py Outdated Show resolved Hide resolved

antalszava reviewed Feb 4, 2020

View reviewed changes

antalszava approved these changes Feb 4, 2020

View reviewed changes

johannesjmeyer and others added 17 commits February 4, 2020 17:25

Refactor state preparation error message

a2581bf

Apply suggestions from code review

a5d51fb

Co-Authored-By: Josh Izaac <josh146@gmail.com>

Introduce _apply_operation method

3475311

Merge branch 'qubit_device_port' of https://www.github.com/XanaduAI/p…

d1907dc

…ennylane-cirq into qubit_device_port

Make CirqDevice abstract

ad81ce7

Make abstract apply methods private

194618c

Remove superfluous pre_apply

c1e4410

Add docstrings for CirqDevice methods

57d374e

Make signature of apply consistend with QubitDevice

ba93996

Improve init test

473efe4

Disable linting only on method level

84b3658

Add _pre_rotation hook

d4a4933

Revert "Add _pre_rotation hook"

bb43ab2

This reverts commit d4a4933.

Add state docstring

c16a217

Remove superfluous pylint comment

09da4a3

Add state tests

2279dd6

Add hardware state test

4fa82b0

johannesjmeyer mentioned this pull request Feb 4, 2020

Bumped version number to 0.2.0 #16

Closed

Increment pennylane requirement in requirements.txt, as the QubitDevi…

3c5fe9f

…ce port has taken place

antalszava merged commit 75c2e52 into master Feb 4, 2020

antalszava reviewed Feb 4, 2020

View reviewed changes

antalszava mentioned this pull request Feb 4, 2020

Increasing the version number to 0.8.0 #21

Merged

co9olguy deleted the qubit_device_port branch May 12, 2020 16:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port PennyLane-Cirq to QubitDevice #19

Port PennyLane-Cirq to QubitDevice #19

johannesjmeyer commented Feb 3, 2020 •

edited

Loading

codecov bot commented Feb 4, 2020 •

edited

Loading

antalszava Feb 4, 2020

johannesjmeyer Feb 4, 2020 •

edited

Loading

johannesjmeyer Feb 4, 2020

johannesjmeyer Feb 4, 2020

josh146 Feb 4, 2020

johannesjmeyer Feb 4, 2020

josh146 Feb 4, 2020

johannesjmeyer Feb 4, 2020

antalszava Feb 4, 2020

antalszava Feb 4, 2020

johannesjmeyer Feb 4, 2020

antalszava Feb 4, 2020

johannesjmeyer Feb 4, 2020

antalszava Feb 4, 2020

johannesjmeyer Feb 4, 2020

johannesjmeyer Feb 4, 2020

antalszava Feb 4, 2020

johannesjmeyer Feb 4, 2020

antalszava left a comment

antalszava Feb 4, 2020

	super().apply(operations, rotations, **kwargs)
	super().apply(operations, rotations=rotations, **kwargs)

		@@ -1,3 +1,3 @@
		pennylane>=0.7
		pennylane>=0.8

Port PennyLane-Cirq to QubitDevice #19

Port PennyLane-Cirq to QubitDevice #19

Conversation

johannesjmeyer commented Feb 3, 2020 • edited Loading

codecov bot commented Feb 4, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

johannesjmeyer Feb 4, 2020 • 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

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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johannesjmeyer commented Feb 3, 2020 •

edited

Loading

codecov bot commented Feb 4, 2020 •

edited

Loading

johannesjmeyer Feb 4, 2020 •

edited

Loading