-
Notifications
You must be signed in to change notification settings - Fork 575
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Circuit cutting: Add final integration tests #2265
Conversation
…pennylane into qcut_remove_unneeded_subgraphs
Codecov Report
@@ Coverage Diff @@
## master #2265 +/- ##
=======================================
Coverage 99.31% 99.31%
=======================================
Files 237 237
Lines 18932 18932
=======================================
Hits 18802 18802
Misses 130 130 Continue to review full report at Codecov.
|
[sc-14724] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Tom, looks good! Tests are passing on my end as well, and seems comprehensive enough. I just left a few comments to clarify some points, then I think it's good to go.
res = cut_circuit() | ||
spy.assert_called_once() | ||
|
||
atol = 1e-2 if shots else 1e-8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to understand this better, using 1e7 shots the best precision we can get is 1e-2? Or is that bound loose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's loose - the test passes with a 1e-3
precision (fails with 1e-4
). Though these tests will be run for every PR and we want them to almost surely pass each time, so 1e-2
seemed safer. In fact, I've additionally added the flaky
decorator to allow the test to potentially fail, it just needs to pass one out of 10 times (updated to one out of 3 times).
def f(params): | ||
qml.BasisState(np.array([1]), wires=[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for not using the qcut decorator in the tests starting from here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we did
@qml.cut_circuit
@qml.qnode(dev)
def f(params):
...
Then it would be hard to separate the uncut circuit from the cut circuit - i.e., f
would necessarily be the cut circuit and we'd have to have another function definition
@qml.qnode(dev)
def f_uncut(params):
...
This would also work, just takes a bit more space (especially since the contents of f
is quite large).
res = circuit(x) | ||
assert np.allclose(res, np.cos(x)) | ||
|
||
def test_circuit_with_trivial_wire_cut(self, use_opt_einsum, mocker): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, would it be too much overhead to add testing in the finite shots case to every test in this class apart from the "complicated circuit"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I'd say so, we need quite a high number of shots to get a decent convergence - 1e7
shots takes the test_simple_cut_circuit
test from <1
second to 20
seconds.
Context:
Adds final integration tests