Adds tests for decomposed frontend gates #173

nquesada · 2019-08-23T21:01:56Z

Context:
Several useful Gaussian gates can be decomposed in terms of beamsplitters and single mode squeezing gates. This PR creates tests that explicitly check that these decompositions are correct
Description of the Change:
Adds new test class TestDecompositionsGaussianGates
Benefits:
Explicit testing for the decompositions derived in the documentation
Possible Drawbacks:

Related GitHub Issues:
#171

…on primitive Gaussian gates

josh146

Great tests @nquesada! Just some minor comments

josh146 · 2019-08-24T07:52:47Z

.github/CHANGELOG.md

@@ -1,3 +1,5 @@
+* Adds new integration tests for the Gaussian gates that are not primitive, i.e. P, CX, CZ, and S2. Addresses issue [#171](https://github.com/XanaduAI/strawberryfields/issues/171)


Suggested change

* Adds new integration tests for the Gaussian gates that are not primitive, i.e. P, CX, CZ, and S2. Addresses issue [#171](https://github.com/XanaduAI/strawberryfields/issues/171)

* Adds new integration tests for the Gaussian gates that are not primitive, i.e. P, CX, CZ, and S2. Addresses issue

[#171](https://github.com/XanaduAI/strawberryfields/issues/171)

Also, this should be moved down the changelog to within the # Release 0.12.0-dev section. Probably under a new subsection ### Improvements.

Thanks, @josh146 . I need to learn some CHANGELOG etiquette :)

strawberryfields/ops.py

josh146 · 2019-08-24T07:55:33Z

tests/integration/test_decompositions_integration.py

+        Vexpected = 0.5 * hbar * Pmat @ np.diag(np.exp([-2 * r, 2 * r])) @ Pmat.T
+        assert np.allclose(Vexpected, state.cov())
+        rexpected = Pmat @ np.array([x1, p1])
+        assert np.allclose(rexpected, state.means())


Should atol=tol and rtol=0 be added to these np.allclose() calls? Especially since tol is being included in the test signature.

yes! Will add it.

josh146 · 2019-08-24T07:56:05Z

tests/integration/test_decompositions_integration.py

+        r = 3
+        x1 = 2
+        p1 = 1.3
+        s = 0.5 * 0


Should this be s = 0.5? Otherwise it looks like this is just testing that Pgate(0) acts as the identity

Yes! Should check again that it works

josh146 · 2019-08-24T07:56:33Z

tests/integration/test_decompositions_integration.py

+        assert np.allclose(state.cov(), Vexpected, atol=tol)
+        rexpected = CXmat @ np.array([x1, x2, p1, p2])
+        # Checks the means are transformed correctly
+        assert np.allclose(state.means(), rexpected, atol=tol)


Missing rtol=0? By default, I think np.allclose() included a non-zero relative tolerance

josh146 · 2019-08-24T07:57:31Z

tests/integration/test_decompositions_integration.py

+            np.exp(1j * phi) * np.sqrt((nbar / (1 + nbar)))
+        ) ** (list(range(n)))
+        diag_elems[-1] = 0
+        assert np.allclose(np.diag(diag_elems), ket, atol=0.2)


josh146 · 2019-08-24T07:57:34Z

tests/integration/test_decompositions_integration.py

+        assert np.allclose(state.cov(), Vexpected, atol=tol)
+        rexpected = CZmat @ np.array([x1, x2, p1, p2])
+        # Checks the means are transformed correctly
+        assert np.allclose(state.means(), rexpected, atol=tol)


tests/integration/test_decompositions_integration.py

josh146 · 2019-08-24T07:58:35Z

tests/integration/test_decompositions_integration.py

+        eng, prog = setup_eng(N)
+        nbar = 1
+        s = np.arcsinh(np.sqrt(1.0))
+        phi = 0 * np.pi / 3


Should phi be zero?

josh146 · 2019-08-24T08:00:39Z

tests/integration/test_decompositions_integration.py

+        n, _ = ket.shape
+        diag_elems = (1 / np.sqrt(1 + nbar)) * (
+            np.exp(1j * phi) * np.sqrt((nbar / (1 + nbar)))
+        ) ** (list(range(n)))


I'm a bit confused why this line works; the LHS of the ** should be a Python float, while the RHS is a list - how come it evaluates as a NumPy array?

Oh, it's because the LHS is of type np.float64.

Maybe it's worth being a bit explicit, and having the exponent be np.arange(n) instead of list(range(n))

Cool! Will do. To be honest I was not sure why it worked.

nquesada

Thanks for the thorough review @josh146 !

…to nonzero values

codecov · 2019-08-26T13:48:47Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d75a078). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #173   +/-   ##
=========================================
  Coverage          ?   97.05%           
=========================================
  Files             ?       43           
  Lines             ?     5398           
  Branches          ?        0           
=========================================
  Hits              ?     5239           
  Misses            ?      159           
  Partials          ?        0

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 d75a078...26ccb52. Read the comment docs.

codecov · 2019-08-26T13:48:47Z

Codecov Report

❗ No coverage uploaded for pull request base (master@d75a078). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #173   +/-   ##
=========================================
  Coverage          ?   97.21%           
=========================================
  Files             ?       40           
  Lines             ?     5273           
  Branches          ?        0           
=========================================
  Hits              ?     5126           
  Misses            ?      147           
  Partials          ?        0

Impacted Files	Coverage Δ
strawberryfields/ops.py	`98.37% <ø> (ø)`

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 d75a078...aebcfe2. Read the comment docs.

Co-Authored-By: Josh Izaac <josh146@gmail.com>

tests/integration/test_decompositions_integration.py

into new_tests

Co-Authored-By: Josh Izaac <josh146@gmail.com>

co9olguy

I have some questions about these tests to resolve before merging

.github/CHANGELOG.md

tests/integration/test_decompositions_integration.py

nquesada · 2019-09-10T15:18:38Z

@co9olguy . I agree with Josh. We could add extra tests to check that the gates are applied in the right order by for instance applying the decomposition we have found and then apply the gate backwards and check that effectively we just applied the identity. However, the spirit of these tests was to check that the operations do what they are supposed to do, for example that the P gate takes x-> x and p-> p+ s x.

co9olguy · 2019-09-11T14:55:05Z

@co9olguy . I agree with Josh. We could add extra tests to check that the gates are applied in the right order by for instance applying the decomposition we have found and then apply the gate backwards and check that effectively we just applied the identity. However, the spirit of these tests was to check that the operations do what they are supposed to do, for example that the P gate takes x-> x and p-> p+ s x.

@nquesada sounds good. I'm happy to leave these tests, which are more concrete (but less amenable to verifying all backends). We should also add the other suggested abstract test strategy (confirm that gates match their decomposing by reversing the arguments and concatenating) before merging.

nquesada · 2019-09-12T14:23:13Z

I think this PR could be merged now. We could make an issue to be addressed in a future PR to check that the gates are applied in the right order and that the "compiled" gate applied backwards reverses them.

co9olguy · 2019-09-12T21:45:26Z

I'm concerned that a magic number tolerance might be masking a possible numerical error. I've set tests to run on default tolerance and observed failures even for cutoff as high as 30 dimensions

co9olguy · 2019-09-12T21:46:36Z

Update: cutoff of 35 seems to pass with default tolerance

tests/integration/test_decompositions_integration.py

co9olguy · 2019-09-14T00:11:00Z

@josh146 I'm inclined to drop the failing test, since the numerics are too sensitive. I've added a secondary test for that gate as well, so we can have some confidence

josh146 · 2019-09-14T00:15:45Z

I'm happy with that, cutoff of 35 is way too high, and the squeezing is almost vacuum to get it to pass with a cutoff of 5.

co9olguy · 2019-09-14T02:30:21Z

@josh146 Ok I removed it. If tests pass, good to merge

nquesada added 5 commits August 23, 2019 14:16

Adds TestDecompositionsGaussianGates to check the correct action of n…

0a3712e

…on primitive Gaussian gates

Adds tests for the missing gates

ea95fa0

Adds tests for the missing gates

f938241

Adds CHANGELOG

2777e88

Adds CHANGELOG

7fd201a

nquesada requested review from co9olguy and josh146 and removed request for co9olguy August 23, 2019 21:02

josh146 requested changes Aug 24, 2019

View reviewed changes

nquesada commented Aug 26, 2019

View reviewed changes

Adds rtol to all the allclose calls and sets all the test parameters …

aebcfe2

…to nonzero values

nquesada and others added 2 commits August 26, 2019 09:51

Updates CHANGELOG

0fc2735

Update strawberryfields/ops.py

b4283a2

Co-Authored-By: Josh Izaac <josh146@gmail.com>

nquesada requested a review from smite August 26, 2019 13:54

smite reviewed Aug 26, 2019

View reviewed changes

tests/integration/test_decompositions_integration.py Outdated Show resolved Hide resolved

nquesada and others added 3 commits August 27, 2019 09:12

Fixes dependency of squeezing parameter on nbar in test_S2gate_fock

7b71349

Merge branch 'new_tests' of https://github.com/XanaduAI/strawberryfields

32c241f

into new_tests

Update strawberryfields/ops.py

2c2a647

Co-Authored-By: Josh Izaac <josh146@gmail.com>

co9olguy requested changes Aug 27, 2019

View reviewed changes

josh146 added the tests label Aug 31, 2019

josh146 changed the title ~~New tests~~ Adds tests for decomposed frontend gates Sep 10, 2019

removes white spaces in lines 1458 an 1459

28d119e

co9olguy added 2 commits September 12, 2019 17:26

make test run correctly with tf and batches

67daeb6

adjusting tolerance to default

725995b

adding requested tests

911ad0b

co9olguy requested a review from josh146 September 12, 2019 22:31

Update .github/CHANGELOG.md

232068d

josh146 approved these changes Sep 12, 2019

View reviewed changes

tests/integration/test_decompositions_integration.py Outdated Show resolved Hide resolved

tests/integration/test_decompositions_integration.py Outdated Show resolved Hide resolved

Apply suggestions from code review

9415ca8

removing test

26ccb52

co9olguy approved these changes Sep 14, 2019

View reviewed changes

josh146 merged commit 65a517e into master Sep 18, 2019

josh146 deleted the new_tests branch September 18, 2019 23:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds tests for decomposed frontend gates #173

Adds tests for decomposed frontend gates #173

nquesada commented Aug 23, 2019

josh146 left a comment

josh146 Aug 24, 2019

josh146 Aug 24, 2019

nquesada Aug 26, 2019

josh146 Aug 24, 2019

nquesada Aug 26, 2019

josh146 Aug 24, 2019

nquesada Aug 26, 2019

josh146 Aug 24, 2019

josh146 Aug 24, 2019

josh146 Aug 24, 2019

josh146 Aug 24, 2019

josh146 Aug 24, 2019

josh146 Aug 24, 2019

nquesada Aug 26, 2019

nquesada left a comment

codecov bot commented Aug 26, 2019 •

edited

Loading

codecov bot commented Aug 26, 2019

co9olguy left a comment

nquesada commented Sep 10, 2019

co9olguy commented Sep 11, 2019

nquesada commented Sep 12, 2019

co9olguy commented Sep 12, 2019

co9olguy commented Sep 12, 2019

co9olguy commented Sep 14, 2019

josh146 commented Sep 14, 2019

co9olguy commented Sep 14, 2019

		@@ -1,3 +1,5 @@
		* Adds new integration tests for the Gaussian gates that are not primitive, i.e. P, CX, CZ, and S2. Addresses issue [#171](https://github.com/XanaduAI/strawberryfields/issues/171)

	* Adds new integration tests for the Gaussian gates that are not primitive, i.e. P, CX, CZ, and S2. Addresses issue [#171](https://github.com/XanaduAI/strawberryfields/issues/171)
	* Adds new integration tests for the Gaussian gates that are not primitive, i.e. P, CX, CZ, and S2. Addresses issue
	[#171](https://github.com/XanaduAI/strawberryfields/issues/171)

Adds tests for decomposed frontend gates #173

Adds tests for decomposed frontend gates #173

Conversation

nquesada commented Aug 23, 2019

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

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

nquesada left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 26, 2019 • edited Loading

Codecov Report

codecov bot commented Aug 26, 2019

Codecov Report

co9olguy left a comment

Choose a reason for hiding this comment

nquesada commented Sep 10, 2019

co9olguy commented Sep 11, 2019

nquesada commented Sep 12, 2019

co9olguy commented Sep 12, 2019

co9olguy commented Sep 12, 2019

co9olguy commented Sep 14, 2019

josh146 commented Sep 14, 2019

co9olguy commented Sep 14, 2019

codecov bot commented Aug 26, 2019 •

edited

Loading