-
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
[Templates] Rewrite layer templates as operations #1163
Conversation
Hello. You may have forgotten to update the changelog!
|
return qml.expval(qml.X(0)) | ||
|
||
|
||
# def circuit_decomposed(*weights): |
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 has to be uncommented once the interferometer issue is solved
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.
Reminder to uncomment!
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.
Ah, thanks, I forgot!
), | ||
(np.array([1, 1]), np.array([0.0 + 0.0j, 0.0 + 0.0j, 0.0 + 0.0j, 1.0 + 0.0j])), | ||
], | ||
) |
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.
Todo: As far as I know we discourage test cases that were just run, and then compared to the output...better to have logical sanity checks?
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.
Having a hard time reviewing this PR with so many changes. Will get back to it with a clearer mind
broadcast(unitary=Displacement, pattern="single", wires=wires, parameters=a_and_phi_a) | ||
@staticmethod | ||
def shape(n_layers, n_wires): | ||
r"""Returns a list of shapes for the 11 parameter tensors. |
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.
😁 😁
Codecov Report
@@ Coverage Diff @@
## master #1163 +/- ##
=======================================
Coverage 98.11% 98.12%
=======================================
Files 145 145
Lines 10943 10971 +28
=======================================
+ Hits 10737 10765 +28
Misses 206 206
Continue to review full report at Codecov.
|
@ixfoduap and @Thenerdstation I know this is a boring PR, but would you mind having a general look? :) |
def expand(self): | ||
|
||
if self.seed is not None: | ||
np.random.seed(self.seed) | ||
|
||
shape = qml.math.shape(self.parameters[0]) | ||
|
||
with qml.tape.QuantumTape() as tape: | ||
|
||
for l in range(self.n_layers): | ||
|
||
i = 0 | ||
while i < shape[1]: | ||
if np.random.random() > self.ratio_imprimitive: | ||
# apply a random rotation gate to a random wire | ||
gate = np.random.choice(self.rotations) | ||
rnd_wire = self.wires.select_random(1) | ||
gate(self.parameters[0][l, i], wires=rnd_wire) | ||
i += 1 | ||
|
||
else: | ||
# apply the entangler to two random wires | ||
if len(self.wires) > 1: | ||
rnd_wires = self.wires.select_random(2) | ||
self.imprimitive(wires=rnd_wires) | ||
return tape |
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.
Do we care at all that expanding the tape will give different circuits each time?
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.
That's the idea of this template...to avoid, the user must feed a seed.
strongly_entangling_layer( | ||
weights=weights[l], wires=wires, r=ranges[l], imprimitive=imprimitive | ||
) | ||
return n_layers, n_wires, 3 |
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.
return n_layers, n_wires, 3 | |
return (n_layers, n_wires, 3) |
Doesn't matter at all but makes it more clear to me it should be treated as a tuple and not 3 separate return args.
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.
Codefactor complains about the unnecessary brakets though...?
QUBIT_DIFFABLE_NONDIFFABLE = [(qml.templates.AmplitudeEmbedding, | ||
{'features': [1 / 2, 1 / 2, 1 / 2, 1 / 2]}, | ||
{'wires': [0, 1], 'normalize': False}, | ||
2), | ||
(qml.templates.AmplitudeEmbedding, | ||
{'features': [1 / 2, 1 / 2, 1 / 2, 1 / 2]}, | ||
{'wires': [0, 1], 'normalize': True}, | ||
2), | ||
(qml.templates.BasisEmbedding, | ||
{}, | ||
{'wires': [0, 1], 'features': [1, 0]}, | ||
2), |
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.
Any reason you removed most of these?
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 was a bit inconsistent here...the test_integration
file will be deleted once all refactor PRs are merged, because the old tests are superseeded. I first deleted things from this file in each PR, then realised I can just leave it and delete in the end. So I'll take care of this!
|
||
assert type(tape.operations[0]) == rotation | ||
assert type(tape.operations[1]) == rotation | ||
assert rotation in [type(gate) for gate in queue] |
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 doesn't check that all gates are rotations, just that a rotation exists in that list.
Can you instead do assert all(type(gate) == rotation for gate in queue)
?
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.
But the queue does not only have rotations...I basically just want to check that the custom gate type is used here...where it is used is tested in the first test...
|
||
res = circuit(weights) | ||
res2 = circuit2(weights) | ||
assert qml.math.allclose(res, res2, atol=tol, rtol=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.
assert qml.math.allclose(res, res2, atol=tol, rtol=0) | |
np.testing.assert_allclose(res, res2, atol=tol, rtol=0) |
This works with TF/Torch/JAX tensor-types too!
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 just checked and works here, but not when a tensor has a grad attached...so will not change it everywhere...
circuit() | ||
circuit2() | ||
|
||
assert np.allclose(dev.state, dev2.state, atol=tol, rtol=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.
Can we instead have the circuits return qml.state()
and check that output instead of relying on dev.state
? In the next quarter, we're going to start enforcing devices be stateless objects/functions, so this will be one of the main things that will be deprecated.
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.
Sorry, I should have left a comment about this (did so in the other PRs): Custom wire labels do not work with qml.state()
. So maybe when this gets deprecated, this test is a good motivation to also fix this limitation. So I'd suggest to leave it like this for now?
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 have to admit that my head hurts trying to review this PR. I'm requesting changes to ensure that Chase's comments are addressed and that a section of the code is uncommented with the resolved interferometer issue.
Otherwise I'm willing to approve to not slow down the process, but I do so without confidence in the correctness of the changes
return qml.expval(qml.X(0)) | ||
|
||
|
||
# def circuit_decomposed(*weights): |
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.
Reminder to uncomment!
Co-authored-by: Chase Roberts <chase@xanadu.ai>
Co-authored-by: Chase Roberts <chase@xanadu.ai>
Thanks so much for the reviews, I know they are hard, but you already picked up a few crucial oversights! Ready for final check! |
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.
YOLO approve
Porting layer templates to operations.
Things that were changed are:
There are 2 outstanding issues:
CVNeuralNetLayers
depends onInterferometer
I thought I also port this template quickly, but there was a problem with the existing gate! I uncommented the really bad test case for now, and another one is failing.ParticleConservingU1
Otherwise ready to review. Sorry, I know it is a nightmare because so much code moved, but just a sanity check regarding the tests and the structure (as well as docstrings) would be great!