Replace old backend with new #67

trbromley · 2021-02-26T22:29:58Z

Fixes #60

codecov · 2021-03-01T21:42:03Z

Codecov Report

Merging #67 (833ace5) into master (c7de986) will increase coverage by 35.74%.
The diff coverage is 88.88%.

@@             Coverage Diff             @@
##           master      #67       +/-   ##
===========================================
+ Coverage   62.29%   98.03%   +35.74%     
===========================================
  Files           5        3        -2     
  Lines         122       51       -71     
===========================================
- Hits           76       50       -26     
+ Misses         46        1       -45

Impacted Files	Coverage Δ
pennylane_lightning/__init__.py	`100.00% <ø> (ø)`
pennylane_lightning/lightning_qubit.py	`97.82% <88.88%> (-2.18%)`	⬇️

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 c7de986...322d80b. Read the comment docs.

trbromley · 2021-03-01T22:38:09Z

pennylane_lightning/lightning_qubit.py

+            if any(isinstance(r, QubitUnitary)for r in rotations):
+                super().apply(operations=[], rotations=rotations)
+            else:
+                self._state = self.apply_lightning(np.copy(self._pre_rotated_state), rotations)


@ThomasLoke - this is an example of a situation where we'd like to pass an arbitrary matrix to the C++ version of apply().

Here, we suppose someone wants to make an arbitrary Hermitian measurement. In PL, this is converted to a diagonalizing unitary and computational basis measurement. The diagonalizing unitary is passed as rotations at the end of the circuit, which in this case requires QubitUnitary.

We don't currently support QubitUnitary because we get an error when serializing op_param, which is a list containing a NumPy array rather than a list of floats.

Being able to pass arbitrary unitaries to the backend would also allow us to directly support QubitUnitary and also new gates like QFT.

Wanted to get your thoughts on how feasible it would be to extend support? Thanks!

Being able to pass arbitrary unitaries to the backend would also allow us to directly support QubitUnitary

For gates that need to be defined by a unitary that can't be known ahead of time, passing in the unitary directly is fine.

also new gates like QFT.

AFAIK, the QFT matrix is just parameterised by its dimension, so I'm not sure I see the need to pass in the matrix directly.

In general, if we can get away with an optimised kernel for gate application, we should. But that's only easily possible for gates that we know ahead of time.

For gates that need to be defined by a unitary that can't be known ahead of time, passing in the unitary directly is fine.

Nice! To clarify, this isn't currently supported right? At least, I get an error if I try:

TypeError: apply(): incompatible function arguments. The following argument types are supported: E 1. (arg0: numpy.ndarray[complex128], arg1: List[str], arg2: List[List[int]], arg3: List[List[float]], arg4: int) -> None

Since we require params to be composed of lists of floats.

AFAIK, the QFT matrix is just parameterised by its dimension, so I'm not sure I see the need to pass in the matrix directly.

In general, if we can get away with an optimised kernel for gate application, we should. But that's only easily possible for gates that we know ahead of time.

Agreed! I guess what I'm getting at is that we just added QFT to PennyLane, and there will inevitably be a gap between gates supported in PL and gates with a defined matrix in the backend. It would be nice for us to have a pipeline to just support any gate that has a matrix method, and we can optionally add in the kernel to the backend if we think it's a popular gate.

To clarify, this isn't currently supported right

Yes, that is correct. In theory, it could be made to work with the current pybind bindings, by encoding the matrix in the vector<double> params as a flattened array of doubles, but there's little benefit of doing so; modifying the bindings is a trivial thing to do.

It would be nice for us to have a pipeline to just support any gate that has a matrix method, and we can optionally add in the kernel to the backend if we think it's a popular gate.

Agreed. Conceivably, we could just delegate any unrecognised gate type to the supplied matrix (and error if that's not provided). However, I am wary about always needing to pay the cost of generating the matrix and passing it to the backend, if we don't have to. I don't expect it to be a large penalty (unless we end up supporting controlled unitary operations with arbitrarily many controls), but I'd rather we do without it if possible.

So I think that at the end of the day, what we really want is not to pass the numpy array of the matrix to the backend directly, but rather to pass a functional that when invoked, generates the numpy array for the matrix. This will allow us to lazily generate the matrix if required, but otherwise delegate to the optimised kernel.

antalszava · 2021-03-01T23:09:29Z

.github/workflows/wheels.yml

@@ -9,7 +9,7 @@ env:
  CIBW_SKIP: "cp27-* cp34-* cp35-* *i686 pp* *win32"

  # Linux specific build settings
-
+  # TODO - remove eigen


Maybe worth merging lines 46 and 47 to check that this PR would indeed fix #60. That separation of tests execution was the hotfix for the issue.

.github/workflows/build.yml

README.rst

albi3ro · 2021-03-04T14:45:50Z

README.rst

@@ -65,15 +65,13 @@ Note that subsequent calls to ``pip install -e .`` will use cached binaries stor
 The following dependencies are required to install PennyLane-Lightning:

 * A C++ compiler, such as ``g++``, ``clang``, or ``MSVC``.


Should we mention we want version 2014 and above now?

So far the C++14 features that are used by the new backend could be replaced by a custom function. So we could stay with C++11 for now (unless another need arises).

antalszava

Looking good so far! 💯 Will do another round after tidying. :)

antalszava · 2021-03-04T15:46:56Z

pennylane_lightning/src/Apply.cpp

 #include "Apply.hpp"
 #include "Gates.hpp"
 #include "StateVector.hpp"
 #include "Util.hpp"
+#include "Util.cpp"


Suggested change

#include "Util.cpp"

antalszava · 2021-03-04T15:48:04Z

pennylane_lightning/src/tests/Makefile

 CC = g++
 CFLAGS = -std=c++11 -g -Wall -fopenmp -fPIC -I/usr/include \
-	-I../include -I$(EIGEN_INCLUDE_DIR) -I$(GOOGLETEST_DIR)/include \


antalszava · 2021-03-04T15:48:58Z

tests/test_apply.py

@@ -1212,42 +1212,3 @@ def test_pauliz_hadamard(self, theta, phi, varphi, monkeypatch, qubit_device_3_w
            - 2 * np.cos(theta) * np.sin(phi) * np.sin(2 * varphi)
        ) / 4
        assert np.allclose(var, expected, atol=tol, rtol=0)
-


This reverts commit 37a9e4c.

This reverts commit 1d38abc.

.github/workflows/build.yml

trbromley · 2021-03-04T15:48:08Z

.github/workflows/deploy.yml

@@ -6,27 +6,11 @@ on:
 env:
  CIBW_SKIP: "cp27-* cp34-* cp35-* *i686 pp* *win32"

-  # Linux specific build settings


Feedback on the changes here is much appreciated!

trbromley · 2021-03-04T15:56:27Z

pennylane_lightning/lightning_qubit.py

@@ -138,7 +116,10 @@ def apply(self, operations, rotations=None, **kwargs):
            self._pre_rotated_state = self._state

        if rotations:
-            self._state = self.apply_lightning(self._pre_rotated_state, rotations)
+            if any(isinstance(r, QubitUnitary)for r in rotations):


Most of the changes in this file are just replacing the old backend with the new, but these lines are also new and separate from the new backend. The idea is to quickly hack-in support for returning Hermitian. This can be done more properly later based on the discussion here.

trbromley · 2021-03-04T16:16:13Z

pennylane_lightning/lightning_qubit.py

        # State preparation is currently done in Python
        if operations:  # make sure operations[0] exists
            if isinstance(operations[0], QubitStateVector):
-                self._apply_state_vector(operations[0].parameters[0], operations[0].wires)
+                self._apply_state_vector(operations[0].parameters[0].copy(), operations[0].wires)


This is due to #81

antalszava · 2021-03-04T16:46:03Z

pennylane_lightning/lightning_qubit.py

+            if any(isinstance(r, QubitUnitary) for r in rotations):
+                super().apply(operations=[], rotations=rotations)
+            else:
+                self._state = self.apply_lightning(np.copy(self._pre_rotated_state), rotations)


This is getting me wondering: each time we call self.apply_lightning, we are doing conversion between Python and C++. If we combined operations and rotations, then we could get away with only a single conversion. In exchange, it would not be so simple to obtain the _pre_rotated_state as before.

Not quite something for this PR, just worth keeping in mind. 🤔

Agreed! Could we just return the pre-rotated state and the post-rotated state? Also I recall some discussion on the rotations API changing, but can't remember the decision?

Could we just return the pre-rotated state and the post-rotated state?

We should be able to, though it might require some thought to 1. still minimize the number of copies made of the state vector 2. keep the code readable.

A naive way would be to make the bound apply C++ function return the pre-rotated state and change the state vector in place to yield the post-rotated state. However, wondering if such a function would lead to much confusion because we'd basically return an intermediate result of the computation.

Also unsure at this moment on how much overhead we're actually incurring from calling on bound C++ functions twice.

Also I recall some discussion on the rotations API changing, but can't remember the decision?

Not quite sure about this one. 🤔

antalszava · 2021-03-04T16:47:37Z

pennylane_lightning/lightning_qubit.py

+            if any(isinstance(r, QubitUnitary) for r in rotations):
+                super().apply(operations=[], rotations=rotations)


So basically this allowed supporting Hermitian, correct?

antalszava

Looks good to me! 💯 🍾

CHANGELOG.md

co9olguy · 2021-03-05T19:49:02Z

🎉

trbromley added 16 commits February 26, 2021 16:57

Update

5e24700

swap

2e0f9bb

Remove benchmark (we can use benchmark suite)

e467754

Remove

21dd041

update setup

1623117

Remove bench

a02030d

Rm old backend

4469b59

Move location

f8dce78

Removals of eigen

b3d6575

Fix

483d173

Fix typo in gate

08a95db

Correct gate type

a878052

Fix Tgate

39f2509

Fix PhaseGate

a62245d

Fix CRY

a1f74f5

Remove extra test

e8c70aa

trbromley mentioned this pull request Mar 1, 2021

Fix some bugs in the gates of the new backend #69

Merged

Hack in Hermitian support

c7861cd

trbromley commented Mar 1, 2021

View reviewed changes

Merge branch 'master' into switch_backend

81c158e

antalszava reviewed Mar 1, 2021

View reviewed changes

trbromley added 8 commits March 2, 2021 08:58

Merge branch 'master' into switch_backend

9ee0e31

Rename

f1e87c3

Add new test empty

4cad99c

Update makefile

b6df95c

Merge branch 'master' into switch_backend

5bfefcd

Update

abb890f

Move getIndicesAfterExclusion

1d38abc

generateBitPatterns

37a9e4c

albi3ro reviewed Mar 4, 2021

View reviewed changes

.github/workflows/build.yml Show resolved Hide resolved

albi3ro reviewed Mar 4, 2021

View reviewed changes

README.rst Show resolved Hide resolved

albi3ro reviewed Mar 4, 2021

View reviewed changes

trbromley added 3 commits March 4, 2021 10:42

Re-add dependencies

e53b5de

Remove eigen from deploy

86e2ff8

Remove eigen from wheels

b93e079

antalszava reviewed Mar 4, 2021

View reviewed changes

trbromley added 7 commits March 4, 2021 10:49

Update Changelog

bb61ea3

Re-add dependency

2f3020f

Typo

79a7295

Revert "generateBitPatterns"

a80a7e9

This reverts commit 37a9e4c.

Revert "Move getIndicesAfterExclusion"

fb7377c

This reverts commit 1d38abc.

Remove

69839e2

Update setup

0b0aec6

trbromley changed the title ~~[WIP] Replace old backend with new~~ Replace old backend with new Mar 4, 2021

Fix CI

0200222

trbromley marked this pull request as ready for review March 4, 2021 16:08

Merge branch 'master' into switch_backend

4f34cd5

trbromley commented Mar 4, 2021

View reviewed changes

trbromley requested review from albi3ro, antalszava and chaserileyroberts March 4, 2021 16:30

antalszava reviewed Mar 4, 2021

View reviewed changes

antalszava approved these changes Mar 4, 2021

View reviewed changes

trbromley commented Mar 5, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

322d80b

trbromley merged commit bed1a2a into master Mar 5, 2021

trbromley deleted the switch_backend branch March 5, 2021 19:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace old backend with new #67

Replace old backend with new #67

trbromley commented Feb 26, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

trbromley Mar 1, 2021

ThomasLoke Mar 2, 2021

trbromley Mar 2, 2021

ThomasLoke Mar 2, 2021 •

edited

Loading

antalszava Mar 1, 2021

albi3ro Mar 4, 2021

antalszava Mar 4, 2021

antalszava left a comment

antalszava Mar 4, 2021

antalszava Mar 4, 2021

antalszava Mar 4, 2021

trbromley Mar 4, 2021

trbromley Mar 4, 2021

trbromley Mar 4, 2021

antalszava Mar 4, 2021 •

edited

Loading

trbromley Mar 4, 2021

antalszava Mar 5, 2021

antalszava Mar 4, 2021

antalszava left a comment

co9olguy commented Mar 5, 2021

		@@ -65,15 +65,13 @@ Note that subsequent calls to ``pip install -e .`` will use cached binaries stor
		The following dependencies are required to install PennyLane-Lightning:

		* A C++ compiler, such as ``g++``, ``clang``, or ``MSVC``.

		if any(isinstance(r, QubitUnitary) for r in rotations):
		super().apply(operations=[], rotations=rotations)

Replace old backend with new #67

Replace old backend with new #67

Conversation

trbromley commented Feb 26, 2021 • edited Loading

codecov bot commented Mar 1, 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

ThomasLoke Mar 2, 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

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

antalszava Mar 4, 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

antalszava left a comment

Choose a reason for hiding this comment

co9olguy commented Mar 5, 2021

trbromley commented Feb 26, 2021 •

edited

Loading

codecov bot commented Mar 1, 2021 •

edited

Loading

ThomasLoke Mar 2, 2021 •

edited

Loading

antalszava Mar 4, 2021 •

edited

Loading