Tape expansion reserves value of the is_sampled attribute #1027

antalszava · 2021-01-22T20:21:55Z

Context:

Devices that do not support an operation try to apply its decomposition. In tape mode this triggers tape expansion. However, in such cases, new tapes are being created and information for the is_sampled attribute is lost.

This causes problems when analytic=True is set under the hood and the QNode returns samples:

import pennylane as qml

qml.enable_tape()

dev = qml.device('default.qubit', wires=1, analytic=True)

dev.operations.remove('T')

@qml.qnode(dev)
def circuit():
    qml.T(wires=0)
    return qml.sample(qml.PauliZ(0))

circuit()

~/pennylane/_qubit_device.py in sample(self, observable)
    681         if isinstance(name, str) and name in {"PauliX", "PauliY", "PauliZ", "Hadamard"}:
    682             # Process samples for observables with eigenvalues {1, -1}
--> 683             return 1 - 2 * self._samples[:, device_wires[0]]
    684 
    685         # Replace the basis state in the computational basis with the correct eigenvalue.

TypeError: 'NoneType' object is not subscriptable

Description of the Change:

Explicitly sets the is_sampled attribute after tape expansion.

Benefits:

Sampling works fine for operations that are not supported.
Usually, the CircuitGraph object can be queried to get this information. However, as the circuit graph is not necessarily constructed after circuit expansion, it is less costly to simply iterate over the observables without constructing the circuit graph.

Possible Drawbacks:
N/A

Related GitHub Issues:
Demo using the Cirq plugin.

…e ret type doesn't get queued)

github-actions · 2021-01-22T20:22:15Z

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

A one-to-two sentence description of the change. You may include a small working example for new features.
A link back to this PR.
Your name (or GitHub username) in the contributors section.

…nylane into tape_expand_sampled

codecov · 2021-01-22T20:39:23Z

Codecov Report

Merging #1027 (141898c) into master (3aa6f47) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1027   +/-   ##
=======================================
  Coverage   97.93%   97.93%           
=======================================
  Files         153      153           
  Lines       11392    11394    +2     
=======================================
+ Hits        11157    11159    +2     
  Misses        235      235

Impacted Files	Coverage Δ
pennylane/tape/operation.py	`98.41% <100.00%> (ø)`
pennylane/tape/tapes/tape.py	`98.33% <100.00%> (+0.01%)`	⬆️

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 3aa6f47...141898c. Read the comment docs.

thisac

Really nice fix @antalszava. 💯 Now the demo works with tape mode!

thisac · 2021-01-22T21:17:26Z

pennylane/tape/qnode.py

+            # Check the observables without needing to create the circuit graph
+            self.qtape.is_sampled = any(obs.return_type == Sample for obs in self.qtape.observables)


Really nice catch @antalszava! 💯

I'm just wondering why the is_sampled method was changed to an attribute. Maybe this is nicer (and a bit more effective) to have it this way rather than always checking for Sample return types in the observables. 🤔

Hmm, so is_sampled is an attribute of QuantumTape, but a property of the CircuitGraph. Here at times, the circuit graph was not constructed, so simply checking the observables directly avoids the overhead of constructing the circuit graph

I mean, an option would be to have QuantumTape.is_sampled a property instead of an attribute, as is the case for CircuitGraph. No need to create the graph in either case; it would be a small change. I'm not really advocating for either though, just a bit curious about the reasoning. 🙂

Hmm, so is_sampled is an attribute of QuantumTape, but a property of the CircuitGraph.

This was likely my fault, I didn't check that it was a property on the graph and just made it an attribute on the tape. However,

this is probably not too much of an issue, since the tape is designed to be (mostly) non-mutable, whereas the circuit graph was very mutable.

It was mostly a temporary fix, I needed the tape to 'look' like a CircuitGraph from a duck-typing perspective, so that existing devices would accept it. With the thinking that soon we would be able to drop this backward compatibility :)

thisac · 2021-01-22T21:19:23Z

tests/tape/tapes/test_tape.py

@@ -840,6 +840,26 @@ def test_expand_tape_multiple_wires_non_commuting(self, ret):
        with pytest.raises(qml.QuantumFunctionError, match="Only observables that are qubit-wise"):
            tape.expand(expand_measurements=True)

+    def test_is_sampled_reserved_after_expansion(self, monkeypatch, mocker):


(thinking aloud) I think this test looks good, but I'm contemplating whether this is too broad of a test (not unit-test-like enough), but I'm not sure how to do it otherwise.

Yeah, think that's a fair point. One thing I found challenging with creating more of a unit test is that a tape is created and expanded in QNode.construct along with other updates (perhaps later down the line we could think about separating these into different methods).

Without being to familiar with writing tests, etc. I would say a good option could simply be to be better at separating the unit tests and the integration tests (and this could perhaps be part of the latter, right?). Then no unit test would be needed for this case at all.

Hmm, it seems that integration tests were not too separated so far for tapes. Might just leave it here for now and we could move it if we start the separation into a new file.

thisac · 2021-01-22T21:24:21Z

pennylane/tape/operation.py

+    # TODO: rename the following usage of JacobianTape to better describe that
+    # it's used for operation expansion


Do you mean simply renaming it to e.g. op_expansion_tape? I'm not against it per se, but I'm thinking it's quite clear already from the method + docstring, and it's only restricted to this quite small method. Maybe this is just something that can be done in this PR (since it's a very small, cosmetic change only) in that case instead of leaving it as a future TODO? 🙂

The comment more so implies that we are creating a JacobianTape which is not necessarily used for jacobian computations. Perhaps we could a) rename JacobianTape or b) create a new type of tape that is more specific to tape expansions.

Oh, I see. In that case, this is a bit more complex. I though you meant renaming the name tape and not the tape Jacobiantape. 😆

Oh good catch @antalszava! I just had a look at the source code for QuantumTape.expand(), and it looks like this doesn't need to be a JacobianTape, you should be able to replace it with a simple QuantumTape with no side effect.

josh146

Thanks Antal for finding this! 💯

Happy for it to be merged as is. One (minor) question though: would it be better to have this at the end of tape expansion, rather than in the QNode? E.g., around here?

pennylane/pennylane/tape/tapes/tape.py

Line 150 in 2ae0263

new_tape._measurements += expanded_tape._measurements

.github/CHANGELOG.md

josh146 · 2021-01-24T14:41:59Z

pennylane/tape/operation.py

+    # TODO: rename the following usage of JacobianTape to better describe that
+    # it's used for operation expansion


Oh good catch @antalszava! I just had a look at the source code for QuantumTape.expand(), and it looks like this doesn't need to be a JacobianTape, you should be able to replace it with a simple QuantumTape with no side effect.

josh146 · 2021-01-24T14:47:18Z

pennylane/tape/qnode.py

+            # Check the observables without needing to create the circuit graph
+            self.qtape.is_sampled = any(obs.return_type == Sample for obs in self.qtape.observables)


Hmm, so is_sampled is an attribute of QuantumTape, but a property of the CircuitGraph.

This was likely my fault, I didn't check that it was a property on the graph and just made it an attribute on the tape. However,

this is probably not too much of an issue, since the tape is designed to be (mostly) non-mutable, whereas the circuit graph was very mutable.

It was mostly a temporary fix, I needed the tape to 'look' like a CircuitGraph from a duck-typing perspective, so that existing devices would accept it. With the thinking that soon we would be able to drop this backward compatibility :)

.github/CHANGELOG.md

Co-authored-by: Josh Izaac <josh146@gmail.com>

…nylane into tape_expand_sampled

antalszava · 2021-01-25T14:53:25Z

Thanks so much for the suggestions!

Changed to have QuantumTape in the expansion logic.
Moved the update of is_sampled to the end of the expansion (instead of the QNode).

antalszava added 13 commits January 22, 2021 09:27

device tests can be run in tape mode

e15940e

queue & patch

7c7d30b

test wires

77d7f9e

Obtain return type within test when tape is already enabled (otherwis…

d046d1b

…e ret type doesn't get queued)

passthru in tape

3429be8

Format

7e8c1f7

args in patched jac

c900537

(revert) args in patched jac

accf225

pylint

ee8c2a9

format

f4b6f80

fix is_sampled for tape expansion

7104e35

test

a13e590

Merge branch 'master' into tape_expand_sampled

18e318d

antalszava added 3 commits January 22, 2021 15:25

no device tests change here

ffb6224

Merge branch 'tape_expand_sampled' of https://github.com/XanaduAI/pen…

30b1697

…nylane into tape_expand_sampled

changelog

df3f9d1

antalszava requested review from josh146 and thisac January 22, 2021 20:31

antalszava added the bug 🐛 Something isn't working label Jan 22, 2021

thisac approved these changes Jan 22, 2021

View reviewed changes

josh146 approved these changes Jan 24, 2021

View reviewed changes

josh146 reviewed Jan 24, 2021

View reviewed changes

.github/CHANGELOG.md Outdated Show resolved Hide resolved

josh146 and others added 6 commits January 24, 2021 22:52

Update .github/CHANGELOG.md

43df09e

Merge branch 'master' into tape_expand_sampled

a535d40

Update .github/CHANGELOG.md

d10a05f

Co-authored-by: Josh Izaac <josh146@gmail.com>

Merge branch 'master' into tape_expand_sampled

66f6ef5

QuantumTape

ad3f845

Merge branch 'tape_expand_sampled' of https://github.com/XanaduAI/pen…

f51d9f0

…nylane into tape_expand_sampled

antalszava added 3 commits January 25, 2021 09:31

update right after tape expansion

4c5db84

formatting

962ac56

Merge branch 'master' into tape_expand_sampled

141898c

antalszava merged commit 2ee5f56 into master Jan 25, 2021

antalszava deleted the tape_expand_sampled branch January 25, 2021 14:53

antalszava mentioned this pull request Mar 8, 2021

Fix is_sampled and its test #1126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tape expansion reserves value of the is_sampled attribute #1027

Tape expansion reserves value of the is_sampled attribute #1027

antalszava commented Jan 22, 2021 •

edited

Loading

github-actions bot commented Jan 22, 2021

codecov bot commented Jan 22, 2021 •

edited

Loading

thisac left a comment

thisac Jan 22, 2021

antalszava Jan 22, 2021

thisac Jan 22, 2021

josh146 Jan 24, 2021

thisac Jan 22, 2021

antalszava Jan 22, 2021

thisac Jan 22, 2021

antalszava Jan 25, 2021

thisac Jan 22, 2021

antalszava Jan 22, 2021

thisac Jan 22, 2021

josh146 Jan 24, 2021

josh146 left a comment

josh146 Jan 24, 2021

josh146 Jan 24, 2021

antalszava commented Jan 25, 2021

		# Check the observables without needing to create the circuit graph
		self.qtape.is_sampled = any(obs.return_type == Sample for obs in self.qtape.observables)

		# TODO: rename the following usage of JacobianTape to better describe that
		# it's used for operation expansion

Tape expansion reserves value of the is_sampled attribute #1027

Tape expansion reserves value of the is_sampled attribute #1027

Conversation

antalszava commented Jan 22, 2021 • edited Loading

github-actions bot commented Jan 22, 2021

codecov bot commented Jan 22, 2021 • edited Loading

Codecov Report

thisac 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

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

josh146 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava commented Jan 25, 2021

antalszava commented Jan 22, 2021 •

edited

Loading

codecov bot commented Jan 22, 2021 •

edited

Loading