Add dimension checking to QubitUnitary. #1439

glassnotes · 2021-06-29T14:48:49Z

Context: Development of #1427 uncovered a bug where QubitUnitary was not checking whether the number of wires matched with the size of the unitary matrix.

Description of the Change: Adds an __init__ method to QubitUnitary to verify dimensionality and unitarity. Changes up the _matrix method accordingly. Also, converts QubitUnitary functions to use qml.math, and separates QubitUnitary tests into separate integration tests for all interfaces.

Benefits: Fixes the bug and clears up ambiguous behaviour such as

qml.QubitUnitary(qml.Hadamard(wires=0).matrix, wires=[0, 1])

which would previously run without warnings.

Possible Drawbacks: None

Related GitHub Issues: None

github-actions · 2021-06-29T14:49:06Z

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.

…eAI/pennylane into fix_qubit_unitary_wires

glassnotes · 2021-06-29T15:32:19Z

tests/test_operation.py

@@ -125,6 +125,9 @@ def test_heisenberg(self, test_class, tol):
    def test_operation_init(self, test_class, monkeypatch):
        "Operation subclass initialization."

+        if test_class == qml.QubitUnitary:


I had to write a separate test for the new __init__ method (see below). I'm actually not sure why it worked before; the issue was that in line 135, the number of wires is computed and then used to construct a list of wires, but QubitUnitary works on any number of wires so it was yielding a list of length 0 and causing the test to fail.

glassnotes · 2021-06-29T15:32:53Z

tests/test_operation.py

+        op = qml.QubitUnitary(U, wires=wires)
+
+        assert op.name == qml.QubitUnitary.__name__
+        assert np.allclose([U], op.data)


This line and the next line are maybe redundant?

codecov · 2021-06-29T15:41:26Z

Codecov Report

Merging #1439 (259482d) into master (63f493d) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1439   +/-   ##
=======================================
  Coverage   98.23%   98.23%           
=======================================
  Files         163      163           
  Lines       12076    12082    +6     
=======================================
+ Hits        11863    11869    +6     
  Misses        213      213

Impacted Files	Coverage Δ
pennylane/ops/qubit.py	`98.65% <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 63f493d...259482d. Read the comment docs.

josh146

Thanks @glassnotes! I left some minor comments in the review itself, but have a bigger request here:

Rather than having a unit test for __init__ (which seems a bit redundant), perhaps it makes more sense to have an integration test, which does the following:

Tests the validation catches incorrect input and allows correct input using NumPy arrays and tensors
Tests that ControlledQubitUnitary continues to avoid the validation?

josh146 · 2021-07-02T04:31:18Z

pennylane/ops/qubit.py

+        # For pure QubitUnitary operations (not controlled), check that the number
+        # of wires fits the dimensions of the matrix
+        if not isinstance(self, ControlledQubitUnitary):
+            U = np.asarray(params[0])


I realize that the previous version of the code does this, but, I only just noticed it 😆

We can't assume that params[0] will be a NumPy array - it could very easily be a TensorFlow or a PyTorch tensor. As a result, best to use qml.math here. E.g.,

U = params[0] shape = qml.math.shape(U)

(and then down below, qml.math.dot and qml.math.conj)

josh146 · 2021-07-02T04:31:38Z

pennylane/ops/qubit.py

+
+    @classmethod
+    def _matrix(cls, *params):
+        return np.asarray(params[0])


Same comment here, this should probably just return params[0]?

josh146 · 2021-07-02T04:32:11Z

tests/test_operation.py

@@ -125,6 +125,9 @@ def test_heisenberg(self, test_class, tol):
    def test_operation_init(self, test_class, monkeypatch):
        "Operation subclass initialization."

+        if test_class == qml.QubitUnitary:


josh146 · 2021-07-02T04:32:26Z

tests/test_operation.py

+        op = qml.QubitUnitary(U, wires=wires)
+
+        assert op.name == qml.QubitUnitary.__name__
+        assert np.allclose([U], op.data)


glassnotes · 2021-07-07T14:08:57Z

Thanks @glassnotes! I left some minor comments in the review itself, but have a bigger request here:

Rather than having a unit test for __init__ (which seems a bit redundant), perhaps it makes more sense to have an integration test, which does the following:
* Tests the validation catches incorrect input and allows correct input using NumPy arrays and tensors

* Tests that `ControlledQubitUnitary` continues to _avoid_ the validation?

@josh146 probably, then, it would be best to split out the QubitUnitary tests into their own test class like the excitation operations, since otherwise that will be quite a big chunk of test living in the TestOperation class. Furthermore, the extensive decomposition tests from #1427 could be included in the new class as well.

integration tests.

glassnotes · 2021-07-07T14:48:44Z

tests/ops/test_qubit_ops.py

-    @pytest.mark.parametrize(
-        "U", [np.array([0]), np.array([1, 0, 0, 1]), np.array([[[1, 0], [0, 1]]])]
-    )
-    def test_qubit_unitary_not_matrix_exception(self, U):


I removed this test because the case is already covered in the unitary_exceptions test, where U[1:] is passed as input.

josh146

@josh146 probably, then, it would be best to split out the QubitUnitary tests into their own test class like the excitation operations, since otherwise that will be quite a big chunk of test living in the TestOperation class. Furthermore, the extensive decomposition tests from #1427 could be included in the new class as well.

I think this makes a lot of sense, it has annoyed me for a while that that test file is so large and hard to add to!

Also new tests look 💯, thanks so much @glassnotes for adding them. It will make the QubitUnitary operation a lot more bug resistant going forward.

josh146 · 2021-07-07T15:13:38Z

tests/ops/test_qubit_ops.py

-    @pytest.mark.parametrize(
-        "U", [np.array([0]), np.array([1, 0, 0, 1]), np.array([[[1, 0], [0, 1]]])]
-    )
-    def test_qubit_unitary_not_matrix_exception(self, U):


Add dimension checking to QubitUnitary.

3a813c2

glassnotes added the bug 🐛 Something isn't working label Jun 29, 2021

Olivia Di Matteo and others added 5 commits June 29, 2021 10:50

Update CHANGELOG.

5295e68

Merge branch 'master' into fix_qubit_unitary_wires

b5f4d4d

Fix test using wrong number of wires.

3534755

Add separate test for init.

a2497b4

Merge branch 'fix_qubit_unitary_wires' of https://github.com/PennyLan…

e971c8f

…eAI/pennylane into fix_qubit_unitary_wires

glassnotes commented Jun 29, 2021

View reviewed changes

glassnotes requested a review from josh146 June 29, 2021 15:36

josh146 requested changes Jul 2, 2021

View reviewed changes

josh146 and others added 3 commits July 2, 2021 12:35

Merge branch 'master' into fix_qubit_unitary_wires

5f4e064

Merge branch 'master' into fix_qubit_unitary_wires

8284a69

Use qml.math within QubitUnitary.

f6847b0

Remove __init__ test; split QubitUnitary tests into separate class of

4e53104

integration tests.

glassnotes commented Jul 7, 2021

View reviewed changes

Update CHANGELOG.

5e6726a

glassnotes requested a review from josh146 July 7, 2021 14:51

Merge branch 'master' into fix_qubit_unitary_wires

d4add4b

josh146 approved these changes Jul 7, 2021

View reviewed changes

Olivia Di Matteo added 3 commits July 7, 2021 11:16

Re-add decomposition tests.

cc335db

Change QubitUnitary error to warning.

368c7b5

Run black.

259482d

glassnotes merged commit a932010 into master Jul 7, 2021

glassnotes deleted the fix_qubit_unitary_wires branch July 7, 2021 18:42

This was referenced Jul 20, 2021

Deprecate Python 3.6 CI check and update qml.QubitUnitary error message in a test PennyLaneAI/pennylane-qulacs#30

Merged

Update qml.QubitUnitary error message in test PennyLaneAI/pennylane-qiskit#144

Merged

antalszava mentioned this pull request Jul 20, 2021

Update qml.QubitUnitary error message in test PennyLaneAI/pennylane-cirq#71

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add dimension checking to QubitUnitary. #1439

Add dimension checking to QubitUnitary. #1439

glassnotes commented Jun 29, 2021 •

edited

Loading

github-actions bot commented Jun 29, 2021

glassnotes Jun 29, 2021

josh146 Jul 2, 2021

glassnotes Jun 29, 2021

josh146 Jul 2, 2021

codecov bot commented Jun 29, 2021 •

edited

Loading

josh146 left a comment

josh146 Jul 2, 2021

josh146 Jul 2, 2021

josh146 Jul 2, 2021

josh146 Jul 2, 2021

glassnotes commented Jul 7, 2021

glassnotes Jul 7, 2021

josh146 Jul 7, 2021

josh146 left a comment

josh146 Jul 7, 2021

Add dimension checking to QubitUnitary. #1439

Add dimension checking to QubitUnitary. #1439

Conversation

glassnotes commented Jun 29, 2021 • edited Loading

github-actions bot commented Jun 29, 2021

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 Jun 29, 2021 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glassnotes commented Jul 7, 2021

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

glassnotes commented Jun 29, 2021 •

edited

Loading

codecov bot commented Jun 29, 2021 •

edited

Loading