-
Notifications
You must be signed in to change notification settings - Fork 586
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
Refactor template tests #409
Conversation
Codecov Report
@@ Coverage Diff @@
## master #409 +/- ##
=========================================
Coverage ? 99.24%
=========================================
Files ? 42
Lines ? 3030
Branches ? 0
=========================================
Hits ? 3007
Misses ? 23
Partials ? 0
Continue to review full report at Codecov.
|
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.
Left a few minor suggestions. My main question is whether the list comprehension which explicitly casts each entry to a numpy array might be masking anything, i.e., shouldn't we also check the cases where the inputs are standard python numbers? Or are there no situations like this, due to how the parameters of a layer are structured?
Co-Authored-By: Nathan Killoran <co9olguy@users.noreply.github.com>
Co-Authored-By: Nathan Killoran <co9olguy@users.noreply.github.com>
…ne into refactor_template_tests
Co-Authored-By: Nathan Killoran <co9olguy@users.noreply.github.com>
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.
Nice work @mariaschuld!
There are two things I think that are missing though:
-
An integration test for the
def circuit(weights):
pattern a user might try -
Gradient tests. Something simple, such as doing
qml.grad()
,torch.backwards()
andtape.grad()
for each of the three interfaces, and making sure this does not raise an exception or returnNone
. Especially working on the VQE module, I found cases where evaluating a circuit with templates would work fine, but then the gradient would raise an exception with TF.Note: with the latter, interface parametrization won't work --- better to have a gradient test class per interface
Oh, another thought - could you also add the test case that you know fails (using the class instead of the decorator), but mark it as |
Working on the grad tests and the xfail one. Isn't the case circuit(weights) covered by test_integration_XX_args in test_templates.py? Only that the number of weights is dynamically inferred. |
Thinking about it - it is major code duplication to add the xfail tests, which would then test something that is completely unrelated to templates. I suggest we rather add a test to qnode itself. Copying the MWE here:
|
From discussion with @josh146 :
|
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.
Thanks @mariaschuld. I only have a couple small questions and comments, but nothing to block merging
…ne into refactor_template_tests
n_wires = len(wires) | ||
if n_wires > 1: | ||
for i in range(n_wires): | ||
imprimitive(wires=[wires[i], wires[(i + r) % n_wires]]) | ||
|
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.
This one is smuggled in from a later PR, but it makes the tests run with 1 wire as well...
class TestParameterIntegration: | ||
""" Integration tests for the parameter generation methods from pennylane.init | ||
and pennylane.templates.layers.""" | ||
interfaces = [('numpy', np.array)] | ||
|
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.
From here on the diff becomes unreadable...I recommend looking at it in the file...
qnode(weights) | ||
assert excinfo.value.args[0] == "StronglyEntanglingLayer requires at least two wires or subsystems to apply " \ | ||
"the imprimitive gates." | ||
|
||
|
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.
See smuggled in fix, now SEL can take 1 wire too
…dding, which after master merge throw exceptions when used second in a circuit
Context: This PR is part of the templates redesign.
Description of the Change: Heavily improves and extends the integration tests. They are now fully automated, so that a developer only has to add the template and inputs to be tested into a list, and it gets automatically tested for different interfaces and different ways to pass parameters.
Benefits: Simpler, more elegant and more extensive tests.
Possible Drawbacks: This PR necessarily contains the small changes in PR #403, which fixes how the templates iterate through weight arrays (making it compatible with tensorflow). I would therefore recommend merging #403 first.
Tests that will be added are integrations for different hyperparameters. However, these tests will fail at the moment, an issue addressed in a separate PR (which will also update the tests).
Related GitHub Issues: -