Support inverse gates #89

trbromley · 2021-03-16T17:56:52Z

No description provided.

pennylane_lightning/src/Apply.hpp

codecov · 2021-03-16T17:58:03Z

Codecov Report

Merging #89 (6e59a18) into master (fd41d8f) will increase coverage by 0.10%.
The diff coverage is 100.00%.

❗ Current head 6e59a18 differs from pull request most recent head 006ff7f. Consider uploading reports for the commit 006ff7f to get more accurate results

@@            Coverage Diff             @@
##           master      #89      +/-   ##
==========================================
+ Coverage   98.03%   98.14%   +0.10%     
==========================================
  Files           3        3              
  Lines          51       54       +3     
==========================================
+ Hits           50       53       +3     
  Misses          1        1

Impacted Files	Coverage Δ
pennylane_lightning/lightning_qubit.py	`97.95% <100.00%> (+0.13%)`	⬆️

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 fd41d8f...006ff7f. Read the comment docs.

trbromley · 2021-03-18T18:05:04Z

pennylane_lightning/lightning_qubit.py

+    kernel_operations = {
+        "PauliX",
+        "PauliY",
+        "PauliZ",
+        "Hadamard",
+        "Hadamard",
+        "S",
+        "T",
+        "RX",
+        "RY",
+        "RZ",
+        "PhaseShift",
+        "Rot",
+        "CNOT",
+        "SWAP",
+        "CZ",
+        "CRX",
+        "CRY",
+        "CRZ",
+        "CRot",
+        "Toffoli",
+        "CSWAP",
+    }


These are the gates that have a kernel supported. Ideally, we would not explicitly set operations and just let it inherit from default.qubit. We'd then have a check in apply_lightning() that maps the operations to QubitUnitary if the operation is not in kernel_operations. In such a way, we're supporting essentially all ops, even without a kernel.

That's a good idea! Adding this set here seems to be out of scope for the PR, would this be related to the upcoming QubitUnitary addition?

trbromley · 2021-03-18T18:08:03Z

tests/test_gates.py

+)
+
+
+@pytest.mark.xfail(strict=True)  # needs support for QubitUnitary


When we add support for arbitrary unitaries, this will be a nice test for gates without a pre-defined kernel.

trbromley · 2021-03-18T18:08:54Z

The performance comparison looks like no change vs master (as expected):

antalszava

Thanks @trbromley! 💯 My main question is related to whether we'd like to have new gate tests here apart from what's there in the device test suite. Also, would we like to unit test the kernel applications?

pennylane_lightning/lightning_qubit.py

antalszava · 2021-03-18T20:36:37Z

pennylane_lightning/lightning_qubit.py

+    kernel_operations = {
+        "PauliX",
+        "PauliY",
+        "PauliZ",
+        "Hadamard",
+        "Hadamard",
+        "S",
+        "T",
+        "RX",
+        "RY",
+        "RZ",
+        "PhaseShift",
+        "Rot",
+        "CNOT",
+        "SWAP",
+        "CZ",
+        "CRX",
+        "CRY",
+        "CRZ",
+        "CRot",
+        "Toffoli",
+        "CSWAP",
+    }


That's a good idea! Adding this set here seems to be out of scope for the PR, would this be related to the upcoming QubitUnitary addition?

pennylane_lightning/src/Apply.cpp

pennylane_lightning/src/Apply.hpp

antalszava · 2021-03-18T20:46:22Z

pennylane_lightning/src/Gates.cpp

@@ -89,7 +98,8 @@ const vector<CplxType> Pennylane::XGate::matrix{
    0, 1,
    1, 0 };

-void Pennylane::XGate::applyKernel(const StateVector& state, const std::vector<size_t>& indices, const std::vector<size_t>& externalIndices) {
+void Pennylane::XGate::applyKernel(const StateVector& state, const std::vector<size_t>& indices, const std::vector<size_t>& externalIndices, bool inverse) {
+    (void)inverse;  // gate is its own inverse


For my own understanding: what is the purpose of this statement?

As far as I understand, it simply avoids the something-something-arg-not-used warning. So it's basically just "using" the inverse arg without doing anything.

Hmm, how come we didn't have such warning for applyKernel? 🤔 We had a thread with Thomas on this here: #85 (comment).

Yes, I think it's motivated for two reasons:

So we won't get the warnings: pennylane_lightning/src/Gates.cpp:101:147: warning: unused parameter ‘inverse’ [-Wunused-parameter]

I like having something that explicitly says the gate is it's own inverse. In other words, we're saying that we know about the inverse argument and don't need to do anything in this case, rather than we just forgot about it.

It'll work either way, so if we're opposed to having this line then I can remove. I don't think this has any performance implications, seems like something more for the compiler.

Got it! It's odd that this warning is emitted 🤔 This is a personal opinion, but the statement looks confusing to me, as it's type casting a boolean to void without any effect. To me, having the parameter unnamed would make up for the unused parameter. The comment is indeed nice to indicate that the gate is its own inverse.

Ok, updated to just have a commend and an unnamed param: fcd1c29

pennylane_lightning/src/Gates.cpp

tests/test_gates.py

antalszava · 2021-03-18T20:57:25Z

tests/test_gates.py

+    return getattr(qml, op_name)
+
+
+@pytest.mark.parametrize("op_name", LightningQubit.kernel_operations)


Isn't this test case covered by running the device test suite? See pennylane/devices/tests/test_gates.py

That's a good point! From a quick look through those tests, I agree that they provide similar coverage to what's here. I'm generally happy to have some redundancy because:

This is directly in lightning.qubit and it is easier for developers to quickly see this failing (often, at least for me, running the dev test suite is more of an afterthought)

I didn't see this exact test in the suite, so maybe there is some edge case covered (not sure)!

Really, having it here explicitly makes me feel more confident that things are definitely working 😆

Agreed 🤔 Calling the device test suite can be overlooked. Could we

Call on the device test suite from the lightning tests (not quite sure about the best way to do this 🤔 )

extend make test with the device test command?

For 1), we could maybe call pl-device-test from one of the test files 🤔

My worry is that we'd start off adding test cases here which will essentially diverge from the device test suite and we'll have to maintain both.

Yes I agree maintainability is important! Because I'm paranoid about double making sure that things work correctly now for lightning.qubit, I still prefer to potentially have some repetition with a benefit of increased confidence. If down the line we see issues with these local tests, we could delete them (and potentially update the dev test suite accordingly).

antalszava · 2021-03-18T20:58:01Z

tests/test_gates.py

+
+
+@pytest.mark.parametrize("op_name", LightningQubit.kernel_operations)
+def test_inverse_unitary_correct(op):


Similarly seems to be covered by the test suite.

thisac

Nice work @trbromley! 🥇

I have the same thought as @antalszava whether we want to add unit tests for the applyKernel functions, although I wouldn't mind if we decided to add them in a separate PR.

thisac · 2021-03-19T14:16:06Z

pennylane_lightning/lightning_qubit.py

-            supports_inverse_operations=False,
+            supports_inverse_operations=True,


pennylane_lightning/lightning_qubit.py

thisac · 2021-03-19T14:43:27Z

pennylane_lightning/src/Gates.cpp

@@ -89,7 +98,8 @@ const vector<CplxType> Pennylane::XGate::matrix{
    0, 1,
    1, 0 };

-void Pennylane::XGate::applyKernel(const StateVector& state, const std::vector<size_t>& indices, const std::vector<size_t>& externalIndices) {
+void Pennylane::XGate::applyKernel(const StateVector& state, const std::vector<size_t>& indices, const std::vector<size_t>& externalIndices, bool inverse) {
+    (void)inverse;  // gate is its own inverse


As far as I understand, it simply avoids the something-something-arg-not-used warning. So it's basically just "using" the inverse arg without doing anything.

pennylane_lightning/src/Gates.cpp

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

trbromley

Thanks @antalszava @thisac!

pennylane_lightning/lightning_qubit.py

trbromley · 2021-03-19T17:26:37Z

pennylane_lightning/src/Gates.cpp

@@ -89,7 +98,8 @@ const vector<CplxType> Pennylane::XGate::matrix{
    0, 1,
    1, 0 };

-void Pennylane::XGate::applyKernel(const StateVector& state, const std::vector<size_t>& indices, const std::vector<size_t>& externalIndices) {
+void Pennylane::XGate::applyKernel(const StateVector& state, const std::vector<size_t>& indices, const std::vector<size_t>& externalIndices, bool inverse) {
+    (void)inverse;  // gate is its own inverse


Yes, I think it's motivated for two reasons:

So we won't get the warnings: pennylane_lightning/src/Gates.cpp:101:147: warning: unused parameter ‘inverse’ [-Wunused-parameter]

I like having something that explicitly says the gate is it's own inverse. In other words, we're saying that we know about the inverse argument and don't need to do anything in this case, rather than we just forgot about it.

It'll work either way, so if we're opposed to having this line then I can remove. I don't think this has any performance implications, seems like something more for the compiler.

pennylane_lightning/src/Gates.cpp

trbromley · 2021-03-19T17:39:17Z

tests/test_gates.py

+    return getattr(qml, op_name)
+
+
+@pytest.mark.parametrize("op_name", LightningQubit.kernel_operations)


That's a good point! From a quick look through those tests, I agree that they provide similar coverage to what's here. I'm generally happy to have some redundancy because:

This is directly in lightning.qubit and it is easier for developers to quickly see this failing (often, at least for me, running the dev test suite is more of an afterthought)

I didn't see this exact test in the suite, so maybe there is some edge case covered (not sure)!

Really, having it here explicitly makes me feel more confident that things are definitely working 😆

trbromley · 2021-03-19T17:42:12Z

Also, would we like to unit test the kernel applications?

It's a good question 🤔 My thinking is that we can move much more quickly when writing Python tests, so if we can access and test from the Python side then we should. I think the Python tests here manage to already provide a nice check of applyKernel(), even if they are not exactly unit tests.

antalszava

Looks good to me! 💯 Awesome that we can now also support inverses 😊

trbromley added 5 commits March 16, 2021 12:53

Expose inverse

b8ab194

Expose inverse to kernels

4b049e7

Add inverses

e066a81

Add to inverses

5bfcc8a

Add missing kernels

a98e895

trbromley commented Mar 16, 2021

View reviewed changes

pennylane_lightning/src/Apply.hpp Outdated Show resolved Hide resolved

Update pennylane_lightning/src/Apply.hpp

9e9ce90

trbromley and others added 13 commits March 16, 2021 14:10

Add inverse in general kernel:

cfe99de

Update capability

680e3fc

Remove .inv from string

0653d4f

Add tests

cffc956

Fix phase shift

00f07cd

Fix

7b3a183

Add Rot kernel

4183b81

Add tests for arbitrary unitary

43a5d07

Remove

04bbe35

Rename

bed0ac8

Fix

1d46c6a

Add to changelog

44ef261

Merge branch 'master' into support_inverse_gates

421a57a

trbromley commented Mar 18, 2021

View reviewed changes

trbromley added 2 commits March 18, 2021 14:28

Try fix codefactor

44a5d16

Probably fix codefactor

5087ff2

trbromley marked this pull request as ready for review March 18, 2021 18:32

trbromley requested review from antalszava and thisac March 18, 2021 18:32

antalszava reviewed Mar 18, 2021

View reviewed changes

thisac approved these changes Mar 19, 2021

View reviewed changes

trbromley and others added 4 commits March 19, 2021 13:13

Apply suggestions from code review

0b095df

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

Remove kernel operations

e56529a

Add to docstrings

8b64d8d

Update one-liners

9023567

trbromley commented Mar 19, 2021

View reviewed changes

trbromley requested a review from antalszava March 19, 2021 18:14

antalszava mentioned this pull request Mar 19, 2021

Support arbitrary gates #91

Closed

Use unnamed param

fcd1c29

antalszava approved these changes Mar 22, 2021

View reviewed changes

Remove kernel_operations

006ff7f

trbromley merged commit 457bc55 into master Mar 22, 2021

trbromley deleted the support_inverse_gates branch March 22, 2021 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support inverse gates #89

Support inverse gates #89

trbromley commented Mar 16, 2021

codecov bot commented Mar 16, 2021 •

edited

Loading

trbromley Mar 18, 2021

antalszava Mar 18, 2021

trbromley Mar 18, 2021

trbromley commented Mar 18, 2021

antalszava left a comment

antalszava Mar 18, 2021

antalszava Mar 18, 2021

thisac Mar 19, 2021

antalszava Mar 19, 2021

trbromley Mar 19, 2021

antalszava Mar 19, 2021

trbromley Mar 22, 2021

antalszava Mar 18, 2021

trbromley Mar 19, 2021

antalszava Mar 19, 2021

trbromley Mar 22, 2021

antalszava Mar 18, 2021

thisac left a comment

thisac Mar 19, 2021

thisac Mar 19, 2021

trbromley left a comment

trbromley Mar 19, 2021

trbromley Mar 19, 2021

trbromley commented Mar 19, 2021 •

edited

Loading

antalszava left a comment

		)


		@pytest.mark.xfail(strict=True) # needs support for QubitUnitary

		return getattr(qml, op_name)


		@pytest.mark.parametrize("op_name", LightningQubit.kernel_operations)



		@pytest.mark.parametrize("op_name", LightningQubit.kernel_operations)
		def test_inverse_unitary_correct(op):

		supports_inverse_operations=False,
		supports_inverse_operations=True,

Support inverse gates #89

Support inverse gates #89

Conversation

trbromley commented Mar 16, 2021

codecov bot commented Mar 16, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trbromley commented Mar 18, 2021

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

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

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

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

trbromley commented Mar 19, 2021 • edited Loading

antalszava left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 16, 2021 •

edited

Loading

trbromley commented Mar 19, 2021 •

edited

Loading