Added a custom gate application for Toffoli in default.qubit #1249

ryanhill1 · 2021-04-22T13:50:55Z

Context:

Custom gate definition for the Toffoli gate

Description of the Change:

In the pennylane/devices/default_qubit.py file, extended the _apply_ops dictionary to contain the "Toffoli": self._apply_toffoli entry
Defined the _apply_toffoli method for DefaultQubit in the same file
Added test cases for applying the Toffoli gate in devices/test_default_qubit.py.

Benefits:

Custom gate definition for the Toffoli gate

Possible Drawbacks:

N/A

Related GitHub Issues:

#1215

antalszava · 2021-04-22T13:56:48Z

Hi @ryanhill1, thanks so much for going for this! 😊 Let us know if anything questions come up in the process.

co9olguy · 2021-04-23T13:41:26Z

Hi @ryanhill1, thanks for the contribution! 🎉

I see you've added code, docstrings, tests, and updated the changelog. Would you say it's ready for code review?

ryanhill1 · 2021-04-23T13:53:12Z

@co9olguy yep! should be ready

antalszava

@ryanhill1 thank you for picking this up! 🎊 Went for a skim through, it seems that the reshaping of the state will need to be adjusted after the computation.

Also, it looks that a file will need to be formatted with black. This can be done by installing black locally with pip install black (the latest version is black-20.8b1 🙂) and then running the following from the directory where PennyLane resides:

black -l 100 pennylane/pennylane/devices/default_qubit.py

Feel free to leave a note for any questions or request another review and will have another look. 🙂

pennylane/devices/default_qubit.py

.github/CHANGELOG.md

tests/devices/test_default_qubit.py

albi3ro

Thanks for the PR! This'll be a great addition.

Right now I think the next steps are:

run black
get tests passing
apply to more general cases
a) any number of wires
b) any ordering of control and target wires

pennylane/devices/default_qubit.py

… review yet

ryanhill1 · 2021-04-30T03:04:22Z

Thank you everyone for all of your feedback! I accidentally pushed some working versions the other day that were super messy, so if you can ignore those and just look at my most recent commit, I think it should be ready for code review! The toffoli function should now work for any number of wires, and any ordering of control and target wires. Plus it is a much simpler implementation than what I had before. I added three test cases that account for both controls being greater than the target, both smaller, and one smaller one greater. I also slightly modified the one and two-qubit test cases to allow us to use the same 4 qubit state across all of them, instead of defining different states for different numbers of wires, which got kind of messy. Let me know any thoughts and/or revisions, I'm happy to make edits as needed.

codecov · 2021-04-30T03:07:36Z

Codecov Report

Merging #1249 (0cd56a0) into master (fe5d1df) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1249   +/-   ##
=======================================
  Coverage   98.11%   98.12%           
=======================================
  Files         146      146           
  Lines       11056    11071   +15     
=======================================
+ Hits        10848    10863   +15     
  Misses        208      208

Impacted Files	Coverage Δ
pennylane/devices/default_qubit.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 fe5d1df...0cd56a0. Read the comment docs.

co9olguy · 2021-04-30T13:34:19Z

Hi @ryanhill1, looks like all checks are passing 🎉

Let us know when this is ready for re-review 🙂

ryanhill1 · 2021-04-30T13:57:34Z

@co9olguy great! In that case it is ready for review

albi3ro

Vast improvement! Thanks for this contribution :)

I'm going to request that the wire order variable gets extracted to pytest parameterization. That way we only need one function. That may also involve using np.tensordot instead of np.einsum.

I did some timing to compare the different methods, dev._apply_unitary versus dev._apply_toffoli. Here's a graph of multiple repetitions of 5000 on 10 wires. Even around the noise on my computer, you can tell your implementation will add some speed and efficiency! Always very exciting to see :)

albi3ro · 2021-04-30T16:55:57Z

tests/devices/test_default_qubit.py

+        state_out = method(self.state, axes=[0, 2, 3])
+        op = op(wires=[0, 2, 3])


Might be less error-prone upon future modification and updates if it's explicit that these two variables are the same thing. Same for the other tests.

Also, we could pull out that variable and parameterize the test over wire order. It would make the code more concise and easier to maintain.

Suggested change

state_out = method(self.state, axes=[0, 2, 3])

op = op(wires=[0, 2, 3])

wires_order = (0,2,3)

state_out = method(self.state, axes=wires_order)

op = op(wires=wires_order)

I've started pulling out the wires variables and parameterizing the tests over the wire order, but I'm having trouble elegantly replacing the einsum functions with tensordot functions. tensordot does not provide a way of reordering the noncontracted dimensions, so I'm instead needing to slice along the control axis, apply tensordot to each slice, and then reassemble the state. But this seems much too similar to the actual implementation to be a fair test. For example, in test_apply_single_qubit_op, if wires_order = [0], then our test state can simply be constructed using state_out_tensordot = np.tensordot(matrix, self.state). But if wires_order = [1], we need to do

result0 = np.tensordot(matrix, self.state[0]) result1 = np.tensordot(matrix, self.state[1]) state_out_tensordot = np.stack([result0, result1])

which gets even more involved for higher dimensions, and therefore is hard to generalize without basically copying the _apply_toffoli implementation. I've tried specifying different axes along which to apply tensordot, but the problem again is the ordering of dimensions. tensordot is fine for einsum operations such as ab,bijk->aijk, because the output is the default ordering. But it seems operations such as ab,ibjk->iajk are not reproducible in tensordot without further specification. And so the general case i.e. making one specification that works for any wires_order, will require if/else statements inside the testing function, which is not ideal. Do you have any ideas?

Edit: I might have just found a workound using np.kron and np.eye to expand the dimensional representation of matrix according to wires_order.

Sorry this didn't work out. The idea was to make the tests simpler, but this sounds like it just makes the tests more complicated.

It was more of a suggestion than a need, so I'm going ahead with changing my review to "approve". Thanks for trying it out anyway 👍

Ok, I think that's right the call. The parameterized version of test_apply_single_qubit_op would have needed to look something like this:

num_wires = 4 state = np.arange(2 ** num_wires) ... single_qubit_wires_order = [ [1], ] ... @pytest.mark.parametrize("op, method", single_qubit_ops) @pytest.mark.parametrize("wires_order", single_qubit_wires_order) def test_apply_single_qubit_op(self, op, method, wires_order, inverse): """Test if the application of single qubit operations is correct.""" state_out = method(self.state.reshape([2] * self.num_wires), axes=wires_order, inverse=inverse) op = op(wires=wires_order) matrix0 = op.inv().matrix if inverse else op.matrix matrix1 = np.kron(np.eye(2 ** wires_order[0]), matrix0) matrix2 = np.kron(matrix1, np.eye(2 ** (self.num_wires - wires_order[0] - 1))) state_out_dot = np.dot(matrix2, self.state).reshape([2] * self.num_wires) assert np.allclose(state_out, state_out_dot)

albi3ro · 2021-04-30T17:59:22Z

pennylane/devices/default_qubit.py

+        cntrl_max = np.argmax(axes[:2])
+        sl_a0 = _get_slice(0, axes[cntrl_max], self.num_wires)
+        sl_a1 = _get_slice(1, axes[cntrl_max], self.num_wires)
+        sl_b0 = _get_slice(0, axes[cntrl_max ^ 1], self.num_wires - 1)


Like the use of ^! very clever way of doing this 👍

albi3ro

Since my idea of parametrizing over wire order actually makes the tests more complicated, instead of less complicated, I'm going to retract that suggestion and change my review to approval 👍

antalszava

@ryanhill1 this is looking 🏆 💯 Had one question about the reason for increasing the number of qubits in the tests, but no major blockers. Will just check back for that, but nearing approval from my side.

antalszava · 2021-05-03T19:48:14Z

pennylane/devices/default_qubit.py

+        cntrl_max = np.argmax(axes[:2])
+        sl_a0 = _get_slice(0, axes[cntrl_max], self.num_wires)
+        sl_a1 = _get_slice(1, axes[cntrl_max], self.num_wires)
+        sl_b0 = _get_slice(0, axes[cntrl_max ^ 1], self.num_wires - 1)


Maybe worth adding cntrl_min = cntrl_max ^ 1 after line cntrl_max and using cntrl_min for readability

just added that change, it does look better!

antalszava · 2021-05-03T19:54:36Z

tests/devices/test_default_qubit.py

+        assert np.allclose(state_out, state_out_einsum)
+
+    @pytest.mark.parametrize("op, method", three_qubit_ops)
+    def test_apply_three_qubit_op_controls_split(self, op, method, inverse):


Thank you for adding this test case!

antalszava · 2021-05-03T19:56:06Z

tests/devices/test_default_qubit.py

@@ -1909,8 +1909,8 @@ class TestApplyOps:
    """Tests for special methods listed in _apply_ops that use array manipulation tricks to apply
    gates in DefaultQubit."""

-    state = np.arange(2 ** 3).reshape((2, 2, 2))
-    dev = qml.device("default.qubit", wires=3)
+    state = np.arange(2 ** 4).reshape((2, 2, 2, 2))


How come we had to go for a device with 4 qubits?

I changed it to a device with 4 qubits so we can test wire orderings in which the two controlled bits and the target bit are not all adjacent to one another. This helps ensure that _apply_toffoli works for any number of wires, not just three.

antalszava

Looks good to me! 💯 Amazing work @ryanhill1 🎉

ryanhill1 added 9 commits April 21, 2021 12:30

first draft implementation of _apply_toffoli

f220751

second draft _apply_toffoli

01ab68b

testing first pass with output

3c843aa

_apply_toffoli is working with forward test case

5dbd7f9

passing all test cases

bf12f0d

deleted comments, working for full pytest. still need documentation

1b88829

documentation done

be46b9d

fixed typo

e2af541

Merge branch 'master' into toffoli

e78225a

ryanhill1 mentioned this pull request Apr 22, 2021

Update CHANGELOG.md #1250

Closed

ryanhill1 added 2 commits April 22, 2021 11:05

Update CHANGELOG.md

208babb

Merge branch 'master' into toffoli

ee6e96d

co9olguy requested review from albi3ro and antalszava April 23, 2021 20:36

antalszava suggested changes Apr 23, 2021

View reviewed changes

pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved

.github/CHANGELOG.md Outdated Show resolved Hide resolved

.github/CHANGELOG.md Outdated Show resolved Hide resolved

antalszava reviewed Apr 23, 2021

View reviewed changes

tests/devices/test_default_qubit.py Outdated Show resolved Hide resolved

albi3ro requested changes Apr 26, 2021

View reviewed changes

pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved

pennylane/devices/default_qubit.py Outdated Show resolved Hide resolved

ryanhill1 and others added 9 commits April 26, 2021 13:49

replaced reshaping with stacking

d9daa23

converting to 4 qubit tests

cf45a76

Merge remote-tracking branch 'origin/toffoli' into toffoli

fdf59b2

Merge branch 'master' into toffoli

798acad

still writing test cases and working on implementation, not ready for…

4244ee1

… review yet

Merge remote-tracking branch 'origin/toffoli' into toffoli

066dc66

Merge branch 'master' into toffoli

401ddd1

passing 21/24 4-qubit 3-permutations

e487604

passing all test cases, documented & ran black

835f5c3

ryanhill1 added 2 commits April 29, 2021 23:08

ran black on test_default_qubit.py

81699ef

wire ordering typo

b870e6b

Merge branch 'master' into toffoli

7638d1c

co9olguy requested review from josh146, antalszava and albi3ro April 30, 2021 14:02

albi3ro requested changes Apr 30, 2021

View reviewed changes

Merge branch 'master' into toffoli

501c847

albi3ro self-requested a review May 3, 2021 17:32

albi3ro approved these changes May 3, 2021

View reviewed changes

Merge branch 'master' into toffoli

da281ab

antalszava reviewed May 3, 2021

View reviewed changes

added cntrl_min = cntrl_max ^ 1 for readability

0cd56a0

antalszava approved these changes May 3, 2021

View reviewed changes

antalszava merged commit 8fe31ad into PennyLaneAI:master May 3, 2021

ryanhill1 deleted the toffoli branch May 3, 2021 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a custom gate application for Toffoli in default.qubit #1249

Added a custom gate application for Toffoli in default.qubit #1249

ryanhill1 commented Apr 22, 2021

antalszava commented Apr 22, 2021

co9olguy commented Apr 23, 2021

ryanhill1 commented Apr 23, 2021

antalszava left a comment

albi3ro left a comment

ryanhill1 commented Apr 30, 2021 •

edited

codecov bot commented Apr 30, 2021 •

edited

co9olguy commented Apr 30, 2021

ryanhill1 commented Apr 30, 2021

albi3ro left a comment

albi3ro Apr 30, 2021

ryanhill1 May 3, 2021 •

edited

albi3ro May 3, 2021

ryanhill1 May 3, 2021

albi3ro Apr 30, 2021

albi3ro left a comment

antalszava left a comment

antalszava May 3, 2021

ryanhill1 May 3, 2021

antalszava May 3, 2021

antalszava May 3, 2021

ryanhill1 May 3, 2021

antalszava left a comment

		state_out = method(self.state, axes=[0, 2, 3])
		op = op(wires=[0, 2, 3])

-        state_out = method(self.state, axes=[0, 2, 3])
-        op = op(wires=[0, 2, 3])
+       wires_order = (0,2,3)
+        state_out = method(self.state, axes=wires_order)
+        op = op(wires=wires_order)

Added a custom gate application for Toffoli in default.qubit #1249

Added a custom gate application for Toffoli in default.qubit #1249

Conversation

ryanhill1 commented Apr 22, 2021

antalszava commented Apr 22, 2021

co9olguy commented Apr 23, 2021

ryanhill1 commented Apr 23, 2021

antalszava left a comment

Choose a reason for hiding this comment

albi3ro left a comment

Choose a reason for hiding this comment

ryanhill1 commented Apr 30, 2021 • edited

codecov bot commented Apr 30, 2021 • edited

Codecov Report

co9olguy commented Apr 30, 2021

ryanhill1 commented Apr 30, 2021

albi3ro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhill1 May 3, 2021 • edited

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

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

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

ryanhill1 commented Apr 30, 2021 •

edited

codecov bot commented Apr 30, 2021 •

edited

ryanhill1 May 3, 2021 •

edited