Update default qubit so that _apply_operation is more functional #1025

trbromley · 2021-01-22T13:11:06Z

This is an update to the internals of default.qubit so that the _apply_operation method accepts an input state and returns an output state. This allows for external functions to interact with the device and request an evolved state - something we need in #1017.

It appears that we do not need to update the tests, since we have not written fully comprehensive unit tests for each method in default.qubit. The current tests use the apply() method, which is one step above _apply_operation, so at least the new changes are still being tested implicitly.

trbromley · 2021-01-22T13:17:52Z

pennylane/devices/default_qubit.py

+            if isinstance(operation, QubitStateVector):
+                self._apply_state_vector(operation.parameters[0], operation.wires)
+            elif isinstance(operation, BasisState):
+                self._apply_basis_state(operation.parameters[0], operation.wires)


These didn't really fit into the idea of applying an operation to a state, since these provide the initial state.

Makes sense. One thing to consider would be to change them such that the state is returned rather than internally updated. This would be useful because then there would be no difference between applying a state preparation and applying an operation in terms of syntax.

self._state = self._apply_state_vector(operation.parameters[0], operation.wires)

We could name the functions self._create_basis_state(state, wires) and self._create_state_vector and have them both return a state variable.

They aren't really "applying" something since it overrides what existed before, if anything existed before.

Thanks! I agree with both suggestions, but suggest we defer them until after release as part of another look at default.qubit.

codecov · 2021-01-22T13:27:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.92%. Comparing base (8792f0f) to head (480dee8).
Report is 2695 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1025      +/-   ##
==========================================
- Coverage   97.92%   97.92%   -0.01%     
==========================================
  Files         151      151              
  Lines       11231    11228       -3     
==========================================
- Hits        10998    10995       -3     
  Misses        233      233

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

trbromley · 2021-01-22T13:51:49Z

pennylane/devices/default_qubit.py

-        if isinstance(operation, QubitStateVector):
-            self._apply_state_vector(operation.parameters[0], wires)
-            return
-
-        if isinstance(operation, BasisState):
-            self._apply_basis_state(operation.parameters[0], wires)
-            return
-


These have been moved to the parent apply() method.

trbromley · 2021-01-22T13:54:13Z

pennylane/devices/default_qubit.py

@@ -527,19 +525,23 @@ def _apply_basis_state(self, state, wires):

        self._state = self._create_basis_state(num)

-    def _apply_unitary(self, mat, wires):
+    def _apply_unitary(self, state, mat, wires):


Ideally we could think about making these as static methods or simply functions, but we do still use self for things like self._tensordot, i.e., interface-dependent calculations.

I second making them static functions 👍
Keeping them methods in the class keeps them organized and contained a bit better, but they should probably be made static methods.

This is a nice idea, which would make things even cleaner. 👍

Note: eventually self._tensordot and all the other ML static methods can be replaced by qml.math.tensordot etc.

But until we do that, we'll likely need to keep them as instance methods 🙁

Oh awesome, am loving the idea to use qml.math!

antalszava

Looks good to me! 💯

antalszava · 2021-01-22T14:20:48Z

pennylane/devices/default_qubit.py

+            if isinstance(operation, QubitStateVector):
+                self._apply_state_vector(operation.parameters[0], operation.wires)
+            elif isinstance(operation, BasisState):
+                self._apply_basis_state(operation.parameters[0], operation.wires)


Makes sense. One thing to consider would be to change them such that the state is returned rather than internally updated. This would be useful because then there would be no difference between applying a state preparation and applying an operation in terms of syntax.

self._state = self._apply_state_vector(operation.parameters[0], operation.wires)

chaserileyroberts

I like it!

josh146 · 2021-01-22T15:55:19Z

I'm pleasantly surprised this required no changes to default.qubit.tf or default.qubit.jax!

albi3ro

With this change, users will also have an easier time playing around in the simulator itself.

"Let's just apply this operation and see how it changes things"
"What's the state look like now, halfway through this circuit?"

I think this will show benefits elsewhere, in addition to just the rewind tape situation.

albi3ro · 2021-01-22T16:17:34Z

pennylane/devices/default_qubit.py


-    def _apply_operation(self, operation):
-        """Applies operations to the internal device state.
+    def _apply_operation(self, state, operation):


I'm mostly going on feeling here, but what about:

_apply_operation(operation, state)

My brain reads that more as "Apply operation onto state" instead of "update state with operation". It just seems a bit easier to process and remember.

Nice that could work! In the interests of time I'd rather stick with the current, but we could consider swapping around post-release.

albi3ro · 2021-01-22T16:26:09Z

pennylane/devices/default_qubit.py

@@ -527,19 +525,23 @@ def _apply_basis_state(self, state, wires):

        self._state = self._create_basis_state(num)

-    def _apply_unitary(self, mat, wires):
+    def _apply_unitary(self, state, mat, wires):


I second making them static functions 👍
Keeping them methods in the class keeps them organized and contained a bit better, but they should probably be made static methods.

thisac

Looks great! I like the functional route we're on! 🌌

thisac · 2021-01-22T16:29:03Z

pennylane/devices/default_qubit.py

@@ -527,19 +525,23 @@ def _apply_basis_state(self, state, wires):

        self._state = self._create_basis_state(num)

-    def _apply_unitary(self, mat, wires):
+    def _apply_unitary(self, state, mat, wires):


This is a nice idea, which would make things even cleaner. 👍

trbromley

Thanks for the comments!

trbromley · 2021-01-22T16:47:20Z

pennylane/devices/default_qubit.py

+            if isinstance(operation, QubitStateVector):
+                self._apply_state_vector(operation.parameters[0], operation.wires)
+            elif isinstance(operation, BasisState):
+                self._apply_basis_state(operation.parameters[0], operation.wires)


Thanks! I agree with both suggestions, but suggest we defer them until after release as part of another look at default.qubit.

trbromley · 2021-01-22T16:48:50Z

pennylane/devices/default_qubit.py

@@ -527,19 +525,23 @@ def _apply_basis_state(self, state, wires):

        self._state = self._create_basis_state(num)

-    def _apply_unitary(self, mat, wires):
+    def _apply_unitary(self, state, mat, wires):


Oh awesome, am loving the idea to use qml.math!

trbromley · 2021-01-22T16:51:48Z

pennylane/devices/default_qubit.py


-    def _apply_operation(self, operation):
-        """Applies operations to the internal device state.
+    def _apply_operation(self, state, operation):


Nice that could work! In the interests of time I'd rather stick with the current, but we could consider swapping around post-release.

trbromley added 3 commits January 22, 2021 08:10

Update

66517cf

Merge

7b7fcbd

fix

fd08137

trbromley commented Jan 22, 2021

View reviewed changes

trbromley added 2 commits January 22, 2021 08:28

Fix pylint

e50972b

Add to changelog

480dee8

trbromley commented Jan 22, 2021

View reviewed changes

trbromley marked this pull request as ready for review January 22, 2021 13:54

trbromley self-assigned this Jan 22, 2021

trbromley added the review-ready 👌 PRs which are ready for review by someone from the core team. label Jan 22, 2021

trbromley requested review from josh146, albi3ro and thisac January 22, 2021 13:55

antalszava self-requested a review January 22, 2021 14:21

antalszava approved these changes Jan 22, 2021

View reviewed changes

chaserileyroberts approved these changes Jan 22, 2021

View reviewed changes

albi3ro reviewed Jan 22, 2021

View reviewed changes

thisac approved these changes Jan 22, 2021

View reviewed changes

trbromley commented Jan 22, 2021

View reviewed changes

josh146 merged commit a7132c8 into master Jan 22, 2021

josh146 deleted the update_apply_operation branch January 22, 2021 17:23

antalszava mentioned this pull request Jan 22, 2021

Apply default qubit state prep api change PennyLaneAI/pennylane-lightning#55

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update default qubit so that _apply_operation is more functional #1025

Update default qubit so that _apply_operation is more functional #1025

trbromley commented Jan 22, 2021 •

edited

Loading

trbromley Jan 22, 2021

antalszava Jan 22, 2021

albi3ro Jan 22, 2021

trbromley Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

trbromley Jan 22, 2021

trbromley Jan 22, 2021

albi3ro Jan 22, 2021

thisac Jan 22, 2021

josh146 Jan 22, 2021

trbromley Jan 22, 2021

antalszava left a comment

antalszava Jan 22, 2021

chaserileyroberts left a comment

josh146 commented Jan 22, 2021

albi3ro left a comment

albi3ro Jan 22, 2021

trbromley Jan 22, 2021

albi3ro Jan 22, 2021

thisac left a comment

thisac Jan 22, 2021

trbromley left a comment

trbromley Jan 22, 2021

trbromley Jan 22, 2021

trbromley Jan 22, 2021

Update default qubit so that _apply_operation is more functional #1025

Update default qubit so that _apply_operation is more functional #1025

Conversation

trbromley commented Jan 22, 2021 • 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

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

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

chaserileyroberts left a comment

Choose a reason for hiding this comment

josh146 commented Jan 22, 2021

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

thisac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

trbromley commented Jan 22, 2021 •

edited

Loading

codecov bot commented Jan 22, 2021 •

edited

Loading