Add CPhaseShiftGate with tests (#112) #114

maliasadi · 2021-07-21T14:09:39Z

Context:
Add support for Controlled Phase Gate (CPhaseShift).

Description of the Change:
Implemented CPhaseShift gate mirroring existing C++ gate structures,
as well as, tested and added to the list of supported gates.

Benefits:
In quantum computing, quantum logic gates are the building blocks of quantum circuits. To make a C++ support for entire gates in pennylane-lightning, this update is adding the missing 2-qubit CPhaseShift gate support.

Possible Drawbacks:

Related GitHub Issues:
(#112) created by @mlxd

codecov · 2021-07-21T14:14:22Z

Codecov Report

Merging #114 (ab6640d) into master (515166e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #114   +/-   ##
=======================================
  Coverage   98.14%   98.14%           
=======================================
  Files           3        3           
  Lines          54       54           
=======================================
  Hits           53       53           
  Misses          1        1

Impacted Files	Coverage Δ
pennylane_lightning/lightning_qubit.py	`97.95% <ø> (ø)`

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 515166e...ab6640d. Read the comment docs.

mlxd

Thanks for the submission @maliasadi
In addition to my listed suggestions, it would also be worth adding a test-case to the Python code (see here for example).
Also, before pushing, it is worth running make format to ensure the C++ formatting conforms to the standards expected. You may need to install clang-format for this to work.

pennylane_lightning/src/Gates.cpp

pennylane_lightning/src/Gates.hpp

pennylane_lightning/src/Gates.cpp

mlxd

Hi @maliasadi thanks again for your work on this. I have some follow-up suggestions. Also, please be sure to run make format before committing and pushing.

tests/test_apply.py

pennylane_lightning/src/tests/TestingUtils.hpp

tests/test_apply.py

mlxd · 2021-07-23T09:05:51Z

Also, just to note, the failing tests on Ubuntu 18.04 are due to an issue with the Github actions image we use. A fix is currently under review, and should solve 2 of those test failures.

maliasadi · 2021-07-23T12:31:17Z

Thank you for your suggestions @mlxd. It's weird that the formatting is failed again! 🤔
Just to let you know, I use ubuntu 18.04 and the following script to check of my changes before each commit:

make clean && \
pip install -e . && \ 
make test && \ 
make test-cpp && \
make format

They are all pass on my side. Do you have any idea why formatting fails here? Also ./bin/format utilizes a subprocess.run feature that is new in python 3.7>, so, if the system doesn't use a proper python3 version, this script will be failed. Could it be a reason here?

mlxd · 2021-07-23T13:21:43Z

Thank you for your suggestions @mlxd. It's weird that the formatting is failed again! thinking
Just to let you know, I use ubuntu 18.04 and the following script to check of my changes before each commit:
make clean && \
pip install -e . && \ 
make test && \ 
make test-cpp && \
make format
They are all pass on my side. Do you have any idea why formatting fails here?

Ah yes, this is likely an older version of clang-format you have on your system. Certain formatting rules changed with v11+, so likely you have 10 or lower on Ubuntu 18.04.

Also ./bin/format utilizes a subprocess.run feature that is new in python 3.7>, so, if the system doesn't use a proper python3 version, this script will be failed. Could it be a reason here?

Yes, this would be expected, as the PennyLane minimum supported Python version is 3.7.

maliasadi · 2021-07-23T14:24:33Z

Ah yes, this is likely an older version of clang-format you have on your system. Certain formatting rules changed with v11+, so likely you have 10 or lower on Ubuntu 18.04.

Yes, you're right! I was using clang-format-10. Thank you @mlxd! I think it's fixed now.

trbromley

Thanks @maliasadi, this looks great! Approved! Though we should coordinate merging this with #113.

trbromley · 2021-07-27T18:04:39Z

.github/CHANGELOG.md

+* Add support for Controlled Phase Gate (CPhaseShift).
+  [(#112)](https://github.com/PennyLaneAI/pennylane-lightning/issues/112)


Suggested change

* Add support for Controlled Phase Gate (CPhaseShift).

[(#112)](https://github.com/PennyLaneAI/pennylane-lightning/issues/112)

* Add support for Controlled Phase Gate (`CPhaseShift`).

[(#114)](https://github.com/PennyLaneAI/pennylane-lightning/pull/114)

Thanks for updating the log file! I update it only because I was asked to do before creating the pull-request.

trbromley · 2021-07-27T18:10:53Z

pennylane_lightning/src/Gates.cpp

+    : shift(std::pow(M_E, CplxType(0, phi))), matrix{1, 0, 0, 0,    0, 1,
+                                                     0, 0, 0, 0,    1, 0,
+                                                     0, 0, 0, shift} {}


Maybe nice to have the matrix part shown in a 4x4 grid to make it visually easy to see what's going on.

(unless clang requires it to be like the above)

This is re-arranged by clang-format, indeed!

Yep, I think we may need to revisit clang formatting rules at a later date to better fix these.

trbromley · 2021-07-27T18:12:33Z

pennylane_lightning/src/Gates.cpp

+
+void Pennylane::CPhaseShiftGate::applyGenerator(
+    const StateVector &state,
+


Suggested change

trbromley · 2021-07-27T18:24:23Z

pennylane_lightning/src/tests/lightning_gates_unittest.cpp

-                                    "CRX", "CRY", "CRZ", "CRot"};
+const vector<string> param_gates = {
+    "RX",  "RY",  "RZ",  "PhaseShift", "Rot",
+    "CRX", "CRY", "CRZ", "CRot",       "ControlledPhaseShift"};


Suggested change

"CRX", "CRY", "CRZ", "CRot", "ControlledPhaseShift"};

"CRX", "CRY", "CRZ", "CRot", "ControlledPhaseShift"};

tests/test_apply.py

mlxd · 2021-07-28T09:40:17Z

As a follow up, I am happy to approve this too. I think it best to allow this PR be merged before #113 as the changes are significant.

mlxd · 2021-07-28T09:48:31Z

Well done on the PR @maliasadi !

maliasadi · 2021-07-28T20:41:57Z

Well done on the PR @maliasadi !

Couldn't done it without your suggestions and supports. Thank you!

ali added 2 commits July 21, 2021 00:16

Add CPhaseShiftGate with tests (#112)

74a4b7b

Add CPhaseShiftGate with tests (#112)

7180d40

josh146 requested review from mlxd and trbromley July 21, 2021 14:21

mlxd requested changes Jul 22, 2021

View reviewed changes

pennylane_lightning/src/Gates.cpp Outdated Show resolved Hide resolved

pennylane_lightning/src/Gates.hpp Show resolved Hide resolved

pennylane_lightning/src/Gates.cpp Show resolved Hide resolved

ali added 2 commits July 22, 2021 11:55

Update support for ControlledPhaseShift Gate (#114)

79594fb

Update test-cases for ControlledPhaseShift Gate (#114)

41bcbeb

maliasadi requested a review from mlxd July 22, 2021 21:42

mlxd requested changes Jul 23, 2021

View reviewed changes

tests/test_apply.py Show resolved Hide resolved

tests/test_apply.py Show resolved Hide resolved

tests/test_apply.py Show resolved Hide resolved

pennylane_lightning/src/tests/TestingUtils.hpp Outdated Show resolved Hide resolved

tests/test_apply.py Outdated Show resolved Hide resolved

Update test-cases and formatting with clang-format-12 (#114)

864ce2e

Merge branch 'master' into master

ab6640d

trbromley approved these changes Jul 27, 2021

View reviewed changes

mlxd approved these changes Jul 28, 2021

View reviewed changes

mlxd merged commit 550ee8a into PennyLaneAI:master Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CPhaseShiftGate with tests (#112) #114

Add CPhaseShiftGate with tests (#112) #114

maliasadi commented Jul 21, 2021

codecov bot commented Jul 21, 2021 •

edited

Loading

mlxd left a comment •

edited

Loading

mlxd left a comment

mlxd commented Jul 23, 2021

maliasadi commented Jul 23, 2021

mlxd commented Jul 23, 2021

maliasadi commented Jul 23, 2021

trbromley left a comment

trbromley Jul 27, 2021

maliasadi Jul 27, 2021

trbromley Jul 27, 2021

trbromley Jul 27, 2021

maliasadi Jul 27, 2021

mlxd Jul 28, 2021

trbromley Jul 27, 2021

trbromley Jul 27, 2021

mlxd commented Jul 28, 2021

mlxd commented Jul 28, 2021

maliasadi commented Jul 28, 2021

		* Add support for Controlled Phase Gate (CPhaseShift).
		[(#112)](https://github.com/PennyLaneAI/pennylane-lightning/issues/112)


		void Pennylane::CPhaseShiftGate::applyGenerator(
		const StateVector &state,

	"CRX", "CRY", "CRZ", "CRot", "ControlledPhaseShift"};
	"CRX", "CRY", "CRZ", "CRot", "ControlledPhaseShift"};

Add CPhaseShiftGate with tests (#112) #114

Add CPhaseShiftGate with tests (#112) #114

Conversation

maliasadi commented Jul 21, 2021

codecov bot commented Jul 21, 2021 • edited Loading

Codecov Report

mlxd left a comment • edited Loading

Choose a reason for hiding this comment

mlxd left a comment

Choose a reason for hiding this comment

mlxd commented Jul 23, 2021

maliasadi commented Jul 23, 2021

mlxd commented Jul 23, 2021

maliasadi commented Jul 23, 2021

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

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

mlxd commented Jul 28, 2021

mlxd commented Jul 28, 2021

maliasadi commented Jul 28, 2021

codecov bot commented Jul 21, 2021 •

edited

Loading

mlxd left a comment •

edited

Loading