Fix bug where non-differentiable NumPy tensors were not unwrapped #941

josh146 · 2020-11-28T07:15:06Z

Context:

The autograd interface was not unwrapping tensors where requires_grad=False. This was intentional, since the np.tensor ndarray subclass, due to ducktyping, should be accepted everywhere that a ndarray would be.

However, recent changes to the np.tensor class have resulted in deviating behaviour from standard ndarrays (e.g., indexing an element returns a scalar tensor rather than a Python float), resulting in some devices unable to process gate arguments correctly.

Description of the Change:

Like the PyTorch and the TF interfaces, constant non-ndarray classes are now always unwrapped before device execution.

Benefits:

np.tensor objects are now never passed to the device - the device will only receive ndarray objects.

Possible Drawbacks:

This extra unwrapping may add some overhead. This overhead may be negligible, but it may not(?).

Further, some devices did work fine with np.tensor gate arguments (e.g., default.qubit), hence why this issue was not noticed here. Perhaps an argument can be made that this fix should be done on the per-device level for devices that need it, to avoid additional overhead? But this would add more maintenance overhead to device developers.

Related GitHub Issues: n/a

…ylane into fix-autograd-unwrapping

josh146 · 2020-11-28T07:18:17Z

pennylane/tape/interfaces/autograd.py

+        if return_arraybox:
+            return params


In order to determine which parameters are trainable, we must not wrap the parameters in an autograd list.

josh146 · 2020-11-28T07:18:44Z

pennylane/tape/interfaces/autograd.py

+        all_params_unwrapped = [p.numpy() if isinstance(p, np.tensor) else p for p in self._all_parameter_values]
+
+        # evaluate the tape
+        self.set_parameters(all_params_unwrapped, trainable_only=False)
        res = self.execute_device(params, device=device)
+        self.set_parameters(self._all_parameter_values, trainable_only=False)


The extra list comprehension and the two new set_parameters might add some overhead, worth benchmarking.

However, this is the same solution the TF and Torch interfaces currently implement, so my (gut) feeling is that the overhead is negligible.

codecov · 2020-11-28T07:24:03Z

Codecov Report

Merging #941 (9eb5353) into master (687fc7c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #941   +/-   ##
=======================================
  Coverage   97.85%   97.85%           
=======================================
  Files         147      147           
  Lines       10124    10127    +3     
=======================================
+ Hits         9907     9910    +3     
  Misses        217      217

Impacted Files	Coverage Δ
pennylane/tape/interfaces/autograd.py	`100.00% <100.00%> (ø)`

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 687fc7c...9eb5353. Read the comment docs.

speller26

This change looks kinda fundamental; is it possible to unit test it?

pennylane/tape/interfaces/autograd.py

speller26 · 2020-11-28T08:15:30Z

pennylane/tape/interfaces/autograd.py

-                if idx in self.trainable_params
-            ]
+        if return_arraybox:
+            return params

        return autograd.builtins.list(params)


When return_arraybox=False, is the previous behavior bugged, or does params look the same? Just curious because it looks like the default behavior is changing too, with params still being passed to autograd.builtins.list.

Hey Cody, this method previously wasn't buggy - while the code here has changed, the (public) logic remains identical.

The main change is that get_parameters() no longer calls self._update_trainable_params(), due to changes in the latter method, which required a bit of a rewrite of the subsequent lines.

So the output of this method should remain the same pre- and post-change.

Sweet, thanks!

speller26 · 2020-11-28T08:20:33Z

pennylane/tape/interfaces/autograd.py

+        self.set_parameters(all_params_unwrapped, trainable_only=False)
        res = self.execute_device(params, device=device)
+        self.set_parameters(self._all_parameter_values, trainable_only=False)


Just for my understanding, what are the two new lines for?

Ah! These two additional lines are the main bugfix.

Previously, res = self.execute_device(params, device=device) would execute the device with the (new) trainable parameters. These would be unwrapped, so that a ndarray would be passed to the device rather than a np.tensor.

However, this did not consider the edge case where non-trainable parameters needed to be unwrapped. This explains why requires_grad=True worked, whereas requires_grad=False didn't!

Here, we are unwrapping all parameters on the tape, executing it, and then reverting to the original parameters (so there are no side-effects for the user)

I think this can be made more efficient in the long-term, by making the two new set_parameter calls the only place where parameter setting happens, and remove the analogous logic (for only trainable parameters) from execute_device.

Co-authored-by: Cody Wang <speller26@gmail.com>

pennylane/tape/interfaces/autograd.py

speller26

Looks great, thanks for tracking this down and fixing it so quickly!

tests/tape/interfaces/test_qnode_autograd.py

trbromley

Thanks @josh146!

pennylane/tape/interfaces/autograd.py

trbromley · 2020-11-30T15:08:55Z

pennylane/tape/interfaces/autograd.py

        res = self.execute_device(params, device=device)
+        self.set_parameters(self._all_parameter_values, trainable_only=False)


This fix comes with a cost of two more calls to set_parameters? Why do we have to reset the parameters back, isn't it ok to just permanently cast the requires_grad tensors to numpy arrays?

Good point. The second call to set_parameters, where the unwrapped parameters are replaced with the wrapped ones, is more to avoid surprising the user. That is, undoing any side effects that tape execution had.

This won't be required if using a QNode, but if using a tape, you might notice the parameter types had changed before/after executing the tape on a device 🤔

Ideally, after this PR is merged we can re-visit and remove all the set_parameter() logic in all three interfaces.

pennylane/tape/interfaces/autograd.py

trbromley

Thanks @josh146

tests/tape/interfaces/test_qnode_autograd.py

pennylane/tape/interfaces/autograd.py

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

trbromley

Thanks @josh146!

pennylane/tape/interfaces/autograd.py

trbromley · 2020-12-08T21:27:37Z

pennylane/tape/interfaces/autograd.py

+            op_idx = self._par_info[p_idx]["p_idx"]
+            params.append(op.data[op_idx])
+
+        return params if return_arraybox else autograd.builtins.list(params)


I think I'm not seeing the subtlety: what was the motivation for updating get_parameters() and _update_trainable_params()?

Previously, get_parameters() would call _update_trainable_params() with every call. I don't think in retrospect this was the right approach;

setting the trainable parameters will be called in general less often than get_parameters() -- the dependency should go the other way. To try and mitigate this overhead, I was setting an instance attribute (params = self._all_parameter_values) which was an ugly side effect that I never liked.

It resulted in a get_parameter() method that differed wildly from the base tape method.

In this PR, this has been corrected:

Now, _update_trainable_params() calls get_parameters(), to get the latest parameters and determine which are trainable.

The contents of the get_parameters() method is now almost identical to the base method, with the exception of the final return statement.

The latter approach is the better approach with less overhead, with the subtlety that _update_trainable_params() needs to get the non-autograd-list sequence of parameters to determine trainability. So the latter approach should have been implemented from the start.

As to why it was changed in this PR; I can't answer that properly. The change below to _execute (to fix the bug) caused the old approach to stop working -- through trial and error I discovered that this approach (the better one) does work with the change, and the entire PL test suite passes.

trbromley

💯

Fix bug where non-differentiable NumPy tensors were not unwrapped

fa44d56

josh146 added the bug 🐛 Something isn't working label Nov 28, 2020

josh146 requested a review from trbromley November 28, 2020 07:15

josh146 added 3 commits November 28, 2020 15:15

Merge branch 'master' into fix-autograd-unwrapping

54707a0

black

395d109

Merge branch 'fix-autograd-unwrapping' of github.com:PennyLaneAI/penn…

0d3e1cb

…ylane into fix-autograd-unwrapping

josh146 commented Nov 28, 2020

View reviewed changes

speller26 reviewed Nov 28, 2020

View reviewed changes

josh146 and others added 2 commits November 28, 2020 16:22

add test

2d185c4

Update pennylane/tape/interfaces/autograd.py

a4c474e

Co-authored-by: Cody Wang <speller26@gmail.com>

speller26 reviewed Nov 28, 2020

View reviewed changes

pennylane/tape/interfaces/autograd.py Outdated Show resolved Hide resolved

fix

fcc9bb1

speller26 approved these changes Nov 28, 2020

View reviewed changes

josh146 commented Nov 28, 2020

View reviewed changes

tests/tape/interfaces/test_qnode_autograd.py Outdated Show resolved Hide resolved

josh146 added 2 commits November 28, 2020 16:47

Update tests/tape/interfaces/test_qnode_autograd.py

29babbc

update changelog

58d8f08

trbromley reviewed Nov 30, 2020

View reviewed changes

Apply suggestions from code review

dcbf7b7

josh146 commented Nov 30, 2020

View reviewed changes

pennylane/tape/interfaces/autograd.py Outdated Show resolved Hide resolved

Apply suggestions from code review

cc48d98

josh146 commented Nov 30, 2020

View reviewed changes

pennylane/tape/interfaces/autograd.py Outdated Show resolved Hide resolved

Update pennylane/tape/interfaces/autograd.py

40f031a

josh146 commented Nov 30, 2020

View reviewed changes

pennylane/tape/interfaces/autograd.py Outdated Show resolved Hide resolved

Update pennylane/tape/interfaces/autograd.py

0a899f0

josh146 commented Nov 30, 2020

View reviewed changes

pennylane/tape/interfaces/autograd.py Outdated Show resolved Hide resolved

josh146 commented Nov 30, 2020

View reviewed changes

pennylane/tape/interfaces/autograd.py Outdated Show resolved Hide resolved

Apply suggestions from code review

c649c77

trbromley mentioned this pull request Nov 30, 2020

Fix bug where NumPy tensors are unsuccessfully passed to the remote device amazon-braket/amazon-braket-pennylane-plugin-python#41

Merged

trbromley reviewed Nov 30, 2020

View reviewed changes

tests/tape/interfaces/test_qnode_autograd.py Outdated Show resolved Hide resolved

pennylane/tape/interfaces/autograd.py Show resolved Hide resolved

josh146 and others added 4 commits December 1, 2020 15:51

Update tests/tape/interfaces/test_qnode_autograd.py

fc4d801

Co-authored-by: Tom Bromley <49409390+trbromley@users.noreply.github.com>

fix jacobian

4662455

remove comment

455bdc8

black

73f7a49

josh146 requested a review from trbromley December 8, 2020 08:20

Merge branch 'master' into fix-autograd-unwrapping

64c2cc8

trbromley approved these changes Dec 8, 2020

View reviewed changes

josh146 added 2 commits December 9, 2020 16:45

Merge branch 'master' into fix-autograd-unwrapping

346d76f

Update pennylane/tape/interfaces/autograd.py

9eb5353

josh146 requested a review from trbromley December 9, 2020 09:07

trbromley approved these changes Dec 9, 2020

View reviewed changes

josh146 merged commit ce7c1de into master Dec 9, 2020

josh146 deleted the fix-autograd-unwrapping branch December 9, 2020 12:35

trbromley mentioned this pull request Jan 8, 2021

Run device comparison test in tape mode #988

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug where non-differentiable NumPy tensors were not unwrapped #941

Fix bug where non-differentiable NumPy tensors were not unwrapped #941

josh146 commented Nov 28, 2020 •

edited

Loading

josh146 Nov 28, 2020

josh146 Nov 28, 2020

josh146 Nov 28, 2020 •

edited

Loading

codecov bot commented Nov 28, 2020 •

edited

Loading

speller26 left a comment

speller26 Nov 28, 2020

josh146 Nov 28, 2020

speller26 Nov 28, 2020

speller26 Nov 28, 2020

josh146 Nov 28, 2020 •

edited

Loading

josh146 Nov 28, 2020

speller26 left a comment

trbromley left a comment

trbromley Nov 30, 2020

josh146 Nov 30, 2020

josh146 Dec 9, 2020

trbromley left a comment

trbromley left a comment

trbromley Dec 8, 2020

josh146 Dec 9, 2020 •

edited

Loading

trbromley left a comment

		res = self.execute_device(params, device=device)
		self.set_parameters(self._all_parameter_values, trainable_only=False)

Fix bug where non-differentiable NumPy tensors were not unwrapped #941

Fix bug where non-differentiable NumPy tensors were not unwrapped #941

Conversation

josh146 commented Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh146 Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Nov 28, 2020 • edited Loading

Codecov Report

speller26 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

Choose a reason for hiding this comment

josh146 Nov 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

josh146 Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

trbromley left a comment

Choose a reason for hiding this comment

josh146 commented Nov 28, 2020 •

edited

Loading

josh146 Nov 28, 2020 •

edited

Loading

codecov bot commented Nov 28, 2020 •

edited

Loading

josh146 Nov 28, 2020 •

edited

Loading

josh146 Dec 9, 2020 •

edited

Loading