Deprecate QNode.draw() and ensure qml.draw() works with the beta QNode #1746

josh146 · 2021-10-13T10:03:22Z

Context: As per issue title.

Description of the Change:

test_circuit_drawer.py is modified to only test the tape datastructure, as testing QNodes is out of scope.
All QNode tests are moved to tests/transforms/test_draw.py, which tests the qml.draw() transform. Additional interface tests are addedd.
A deprecation warning is added to QNode.draw()
The qml.draw() tests are updated to work with the beta QNode.

Benefits: As per issue title.

Possible Drawbacks: None

Related GitHub Issues: n/a

josh146 · 2021-10-13T10:07:58Z

[sc-9748]

codecov · 2021-10-13T10:26:32Z

Codecov Report

Merging #1746 (c6a2b1a) into master (82ffc26) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1746   +/-   ##
=======================================
  Coverage   99.22%   99.22%           
=======================================
  Files         209      209           
  Lines       15616    15617    +1     
=======================================
+ Hits        15495    15496    +1     
  Misses        121      121

Impacted Files	Coverage Δ
pennylane/qnode.py	`98.74% <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 82ffc26...c6a2b1a. Read the comment docs.

antalszava

Overall looking great! 👍 Left some questions & suggestions. 🙂

doc/releases/changelog-dev.md

pennylane/qnode.py

antalszava · 2021-10-15T17:31:46Z

tests/circuit_drawer/test_circuit_drawer.py

+            " 0: ──RY(1)──H──RZ(2)──╭┤ ⟨X ⊗ X ⊗ X⟩ ╭┤ ⟨X ⊗ X⟩ \n"
+            + " 1: ───────────────────├┤ ⟨X ⊗ X ⊗ X⟩ │┤         \n"
+            + " 2: ───────────────────╰┤ ⟨X ⊗ X ⊗ X⟩ │┤         \n"
+            + " 3: ────────────────────┤             ╰┤ ⟨X ⊗ X⟩ \n"


We seem to have drawn circuits by including the diagonalizing gates and showing Z measurements. For tapes, it seems to make sense not to do that because the device is yet to be determined.

Would we not like to draw it as such for qml.draw? In specific, logically it would make sense to draw the exact circuit that is run on a device, right?

Simulators that support the observables in the measurements: no diagonalizing gates

Devices that do not support the observables in the measurements (e.g., HW devices): diagonalizing gates and Z measurements

Hey @antalszava, my understanding was that the existing behaviour was incorrect, but let me know if I am wrong! E.g., using the current master of PennyLane, a single expectation value is left as is:

dev = qml.device("default.qubit", wires=2) @qml.qnode(dev) def circuit(x): qml.RX(x, wires=0) qml.CNOT(wires=[0, 1]) return qml.expval(qml.PauliX(wires=0)) >>> print(qml.draw(circuit)(0.6)) 0: ──RX(0.6)──╭C──┤ ⟨X⟩ 1: ───────────╰X──┤

As a result, I would expect multiple expectation values to also be left as is, but this is not the case.

Hmm interesting 🤔 This does seem to come from the tape though, rather than the circuit drawer 🤔

We have the diagonalizing gates applied in the circuit when we have multiple measurements:

import pennylane as qml dev = qml.device("default.qubit", wires=4) @qml.qnode(dev) def circuit(x): return qml.expval(qml.PauliX(wires=0)) circuit(0.6) print(circuit.qtape.operations) dev = qml.device("default.qubit", wires=4) @qml.qnode(dev) def circuit(x): return [ qml.expval(qml.PauliX(wires=[0]) @ qml.PauliX(wires=[1]) @ qml.PauliX(wires=[2])), qml.expval(qml.PauliX(wires=[0]) @ qml.PauliX(wires=[3])), ] circuit(0.6) print(circuit.qtape.operations)

[] [RY(-1.5707963267948966, wires=[0]), RY(-1.5707963267948966, wires=[1]), RY(-1.5707963267948966, wires=[2]), RY(-1.5707963267948966, wires=[3])]

Is this a bug that's in the current QNode?

Yes, this is super weird 🤔

I must say, I'm not sure it is a bug per se (since it isn't affecting the circuit output), but it is definitely an inconsistency that appears to be picked up by the circuit drawer.

I'm also not sure why the new QNode doesn't share this inconsistency. Perhaps because, in the new QNode, the expansion has been moved to the device, and doesn't happen during construction?

Could be. If this does happen with the new QNode though (just further down in the pipeline), then I'm not sure it's ideal. We're in practice applying gates which we could get away with not doing. For example for default.qubit we are computing expvals using the eigenvalues of the observable (instead of applying rotations).

For example for default.qubit we are computing expvals using the eigenvalues of the observable (instead of applying rotations).

I thought default.qubit is doing the rotations? Since when computing \sum_i lambda_i probs_i, the probability vector has been rotated.

I thought default.qubit is doing the rotations?

Do you mean that it always applies the diagonalizing gates of an observable in the circuit?

Since when computing \sum_i lambda_i probs_i, the probability vector has been rotated.

Isn't it the case that \sum_i lambda_i probs_i can be used in two ways?

Each lambda_i is the eigenvalue of the observable that was specified and probs_i are computed using the pre-measurement state;

Each lambda_i is the eigenvalue of the PauliZ operator and probs_i is using the state that was evolved using the diagonalizing gates.

Both 1. and 2. can be used for default.qubit, but 1. should be more efficient (because usually we know the eigenvalues in advance).

On another note, it seems that we apply the diagonalizing gates using default.qubit when multiple observables are being evaluated on the same wire.

I'm not sure that is the case 🤔 PauliX, PauliY, and PauliZ all have the same eigenvalues, (-1, 1). The only thing that distinguishes them are the rotations, so the rotations must always occur.

Here,

pennylane/pennylane/_qubit_device.py

Line 771 in 3c5863a

prob = self.probability(wires=observable.wires)

I imagine that self.probabilities() is returning the rotated probabilities?

Oh boy, you're right, yes!

It seems that the diagonalizing gates are being applied as tape operations (instead of as diagonalizing gates) when we have multiple observables being evaluated on the same wire:

import pennylane as qml dev = qml.device("default.qubit", wires=4) @qml.qnode(dev) def circuit(x): return qml.expval(qml.PauliX(wires=0)) circuit(0.6) print("Ops: \n", circuit.qtape.operations, "\nDiagonalizing gates: \n", circuit.qtape.diagonalizing_gates) dev = qml.device("default.qubit", wires=4) @qml.qnode(dev) def circuit(x): return [ qml.expval(qml.PauliX(wires=[0]) @ qml.PauliX(wires=[1]) @ qml.PauliX(wires=[2])), qml.expval(qml.PauliX(wires=[0]) @ qml.PauliX(wires=[3])), ] circuit(0.6) print("\nOps: \n", circuit.qtape.operations, "\nDiagonalizing gates: \n", circuit.qtape.diagonalizing_gates)

Ops: [] Diagonalizing gates: [Hadamard(wires=[0])] Ops: [RY(-1.5707963267948966, wires=[0]), RY(-1.5707963267948966, wires=[1]), RY(-1.5707963267948966, wires=[2]), RY(-1.5707963267948966, wires=[3])] Diagonalizing gates: []

The rotating operations are pushed into tape._ops here.

Ah, nice 🕵️-ing!

Adding the rotations to tape._ops, and changing the observables to Z, is the correct approach, so it looks like everything is working as intended. Albeit, this doesn't match what happens when you have single measurements per wire!

Perhaps @trbromley can add more details here, since I believe he originally added this. It could be the case that this was a workaround. For example, if you have

return expval(X(0)), expval(X(0) @ Z(1))

and attempted to do it using diagonalizing_gates, I think you would run into an issue where rotations will be applied for both X(0) observables, resulting in too many rotations being appended.

tests/circuit_drawer/test_circuit_drawer.py

tests/tape/test_qnode.py

antalszava · 2021-10-15T17:39:46Z

tests/transforms/test_draw.py

@@ -29,7 +29,89 @@ def test_drawing():

    dev = qml.device("default.qubit", wires=2)

-    @qml.qnode(dev, interface="autograd")
+    @qml.beta.qnode(dev, interface="autograd")


Until the swapping of QNodes, would we want both of them to pass these tests? @qml.QNode would raise a warning.

I don't think it is worth it, to be honest 🤔 Partly because qml.draw() simply calls qnode.qtape.draw, which is the identical for both QNodes (and is still tested in both cases)

tests/transforms/test_draw.py

anthayes92

Great to see this update @josh146! Just a couple of suggestions and questions

anthayes92 · 2021-10-15T18:18:20Z

tests/transforms/test_draw.py

+    wires works correctly when there are more observables than the number of
+    observables for any wire.


This wording for the observables is a little unclear, I’m not sure what is being tested here beyond multiple measurements

I agree 🤔 @antalszava, I think you might have originally written this docstring when fixing a drawing bug, do you recall what this means?

Yep! We have 3 measurements in the return statement, whereas there is at most 2 observables that act on any of the wires from these measurements:

Wire 0: qml.PauliZ(0), qml.PauliZ(0) from the qml.PauliZ(0) @ qml.PauliZ(1) tensor product

Wire 1: qml.PauliZ(1), qml.PauliZ(1) from the qml.PauliZ(0) @ qml.PauliZ(1) tensor product

tests/transforms/test_draw.py

Co-authored-by: antalszava <antalszava@gmail.com>

antalszava

Looks good to me! 🥳 Nice that we'll have one functional way for drawing circuits. 🙂

Deprecate QNode.draw() and ensure qml.draw() works with the beta QNode

9f7f218

josh146 added the review-ready 👌 PRs which are ready for review by someone from the core team. label Oct 13, 2021

josh146 requested a review from mariaschuld October 13, 2021 10:03

Add interface tests

5898835

add another test

718ccb7

josh146 added WIP 🚧 Work-in-progress and removed WIP 🚧 Work-in-progress labels Oct 13, 2021

josh146 requested review from anthayes92 and removed request for mariaschuld October 14, 2021 10:17

josh146 added 2 commits October 14, 2021 18:19

Merge branch 'master' into beta-qnode-draw

0fc4f5a

Merge branch 'master' into beta-qnode-draw

c76143f

josh146 mentioned this pull request Oct 15, 2021

Update all instances of qnode.draw to qml.draw() #1750

Merged

josh146 requested a review from antalszava October 15, 2021 06:30

josh146 added 4 commits October 15, 2021 14:31

Merge branch 'master' into beta-qnode-draw

5fc2f97

more tests

e589483

Merge branch 'master' into beta-qnode-draw

1cd385b

Merge branch 'master' into beta-qnode-draw

d9e726e

antalszava reviewed Oct 15, 2021

View reviewed changes

anthayes92 approved these changes Oct 15, 2021

View reviewed changes

antalszava and others added 5 commits October 15, 2021 14:53

Merge branch 'master' into beta-qnode-draw

ff0d1bc

Apply suggestions from code review

8639239

Co-authored-by: antalszava <antalszava@gmail.com>

Merge branch 'master' into beta-qnode-draw

038d287

Update test_circuit_drawer.py

1c7ddf5

black

c6a2b1a

antalszava approved these changes Oct 18, 2021

View reviewed changes

josh146 merged commit 8de4f0c into master Oct 18, 2021

josh146 deleted the beta-qnode-draw branch October 18, 2021 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate QNode.draw() and ensure qml.draw() works with the beta QNode #1746

Deprecate QNode.draw() and ensure qml.draw() works with the beta QNode #1746

josh146 commented Oct 13, 2021 •

edited

Loading

josh146 commented Oct 13, 2021

codecov bot commented Oct 13, 2021 •

edited

Loading

antalszava left a comment

antalszava Oct 15, 2021

josh146 Oct 18, 2021 •

edited

Loading

antalszava Oct 18, 2021

josh146 Oct 18, 2021

antalszava Oct 18, 2021 •

edited

Loading

josh146 Oct 18, 2021

antalszava Oct 18, 2021

josh146 Oct 18, 2021

antalszava Oct 18, 2021

josh146 Oct 19, 2021

antalszava Oct 15, 2021

josh146 Oct 16, 2021

anthayes92 left a comment

anthayes92 Oct 15, 2021

josh146 Oct 16, 2021

antalszava Oct 18, 2021 •

edited

Loading

antalszava left a comment

		wires works correctly when there are more observables than the number of
		observables for any wire.

Deprecate QNode.draw() and ensure qml.draw() works with the beta QNode #1746

Deprecate QNode.draw() and ensure qml.draw() works with the beta QNode #1746

Conversation

josh146 commented Oct 13, 2021 • edited Loading

josh146 commented Oct 13, 2021

codecov bot commented Oct 13, 2021 • edited Loading

Codecov Report

antalszava left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josh146 Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antalszava Oct 18, 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

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

anthayes92 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 Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

antalszava left a comment

Choose a reason for hiding this comment

josh146 commented Oct 13, 2021 •

edited

Loading

codecov bot commented Oct 13, 2021 •

edited

Loading

josh146 Oct 18, 2021 •

edited

Loading

antalszava Oct 18, 2021 •

edited

Loading

antalszava Oct 18, 2021 •

edited

Loading