-
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
Lie gradient flow optimizer #1911
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1911 +/- ##
=======================================
Coverage 98.80% 98.81%
=======================================
Files 225 226 +1
Lines 17144 17233 +89
=======================================
+ Hits 16939 17028 +89
Misses 205 205
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.
Nice work @therooler! I've gone through and left some first-pass comments and questions, mostly just for my own understanding :)
pennylane/optimize/lie_gradient.py
Outdated
out_plus = qml.execute( | ||
circuit_plus, self.circuit.device, gradient_fn=qml.gradients.param_shift | ||
) | ||
out_min = qml.execute( | ||
circuit_min, self.circuit.device, gradient_fn=qml.gradients.param_shift | ||
) |
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.
@therooler you could optimize this further by executing all circuits at once,
out = qml.execute(circuits, ...)
for out_plus, out_min in out:
...
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.
Hmm this
out = qml.execute(
circuits, self.circuit.device, gradient_fn=None
)
for out_plus, out_min in out:
# depending on the length of the grouped observable, store the omegas in the array
omegas[idx : idx + len(out_plus[0]), :] = 0.5 * (
np.array(out_plus).T - np.array(out_min).T
)
idx += len(out_plus[0])
return np.dot(self.coeffs, omegas)
throws an error:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../pennylane/optimize/lie_gradient.py:246: in step
self.step_and_cost(
../../pennylane/optimize/lie_gradient.py:197: in step_and_cost
omegas = self.get_omegas()
../../pennylane/optimize/lie_gradient.py:321: in get_omegas
out = qml.execute(
../../pennylane/interfaces/batch/__init__.py:313: in execute
with qml.tape.Unwrap(*tapes):
../../pennylane/tape/unwrap.py:82: in __enter__
stack.enter_context(
../../../../anaconda3/envs/pennylane/lib/python3.8/contextlib.py:425: in enter_context
result = _cm_type.__enter__(cm)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <pennylane.tape.unwrap.UnwrapTape object at 0x7f7ac8ed0b20>
def __enter__(self):
> self._original_params = self.tape.get_parameters(trainable_only=False)
E AttributeError: 'tuple' object has no attribute 'get_parameters'
../../pennylane/tape/unwrap.py:132: AttributeError
`
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 my mistake 🤦 This is because the qml.execute
only accepts a flat list of tapes. You would need to do something like this:
c_plus, c_minus = list(zip(*circuits))
out = qml.execute(c_plus + c_minus, ...)
o_plus = out[:len(out) // 2]
o_minus = out[len(out) // 2:]
for out_plus, out_min in zip(o_plus, o_minus):
...
The main advantage here is that you are only calling execute
once, so if you are using a cloud-based device, only one job is submitted over the API.
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.
Hmm I'm running into some issues here because the returned circuit outputs have different lengths based on the Pauli groupings. Should be able to fix this...
Co-authored-by: Josh Izaac <josh146@gmail.com>
[sc-9759] |
I implemented the approximate and exact (non-trotterized) lie gradient. The optimizer takes a single arg to turn of exact or Trotterized evolution, and a restriction of the lie gradient to some subspace can now be achieved by passing a qml.Hamitonian object that spans this subspace. I still need to add more tests to ensure that everything is working properly, but after that it should be ready for another review I think. |
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.
@therooler great improvement, the new logic looks 💯! Very clean. I left a lot of comments, but the PR is in a really good spot --- my comments mostly relate to final touch ups such as the documentation. In particular, ensuring that the usage, behaviour, advantages and restrictions of the new LieGradientOptimizer is properly motivated and explained :)
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
Hi, I think I managed to address most of your comments:
Thanks for the great feedback 😄 . |
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.
@therooler this PR is now looking in great shape, and all the documentation renders very nicely! Great job.
This is essentially merge-ready, with only two things I noticed that should be done/considered:
-
Don't forget to
black -l 100 pennylane tests
and ensure all the codefactor/pylint errors are resolved! -
It would be nice to consider and document (with an example) how a user extracts the optimized circuit structure/QNode.
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
I checked the docs, did the pylint and black. I think it all looks good! Oh and I changed everything to Lie algebra optimizer with Riemannian gradients. |
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.
@therooler 💯 Looks very polished, will be very nice to have this in!
I'm happy to approve, this is a great feature to have in in its current form.
I only have a couple of things that might be worth considering, either here, on in a separate PR:
- It seems to be trivial to support parametrized QNodes
- Maybe out of scope for this PR, but I would be curious to know if this works with other autodiff frameworks such as TF or JAX. While the autodiff functionality of those frameworks is not needed, what could be cool is using their just-in-time compilation support to speed up the optimization.
def step_and_cost(self): | ||
r"""Update the circuit with one step of the optimizer and return the corresponding | ||
objective function value prior to the step. | ||
|
||
Returns: | ||
tuple[.QNode, float]: the optimized circuit and the objective function output prior | ||
to the step. | ||
""" | ||
# pylint: disable=not-callable | ||
|
||
cost = self.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.
@therooler this is my fault for not noticing this earlier, but:
I think it should be trivial to support parametrized QNodes by simply doing
def step_and_cost(self): | |
r"""Update the circuit with one step of the optimizer and return the corresponding | |
objective function value prior to the step. | |
Returns: | |
tuple[.QNode, float]: the optimized circuit and the objective function output prior | |
to the step. | |
""" | |
# pylint: disable=not-callable | |
cost = self.circuit() | |
def step_and_cost(self, *args, **kwargs): | |
r"""Update the circuit with one step of the optimizer and return the corresponding | |
objective function value prior to the step. | |
Returns: | |
tuple[.QNode, float]: the optimized circuit and the objective function output prior | |
to the step. | |
""" | |
# pylint: disable=not-callable | |
cost = self.circuit(*args, **kwargs) |
(and similar for self.step()
).
This should be a minor change, but potentially a big quality-of-life improvement for users, and bring the optimizer in line with other optimizers!
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.
(Note that this is not required for approval, as I have already approved)
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.
Let me see if this breaks things.
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.
So the issue here is that I need to construct the circuit in init, otherwise things break. I haven't been able to figure out how make things work without that.
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.
Oh of course, thanks for exploring this!
suggestions josh Co-authored-by: Josh Izaac <josh146@gmail.com>
Context:
Implementation of Lie gradient flow on quantum circuits.
Description of the Change:
New optimizer is added
The lie gradient optimizer grows the circuit by appending unitaries corresponding to the Lie gradient to the circuit.
This is currently done via a @qml.qfunc_transform and this works reasonably well. I wrote a couple of tests to make sure that calling opt.step() works and that what is being calculated is checked against what you would get with doing everything in numpy.
One important issue that I'm having is calculating the cost after the circuit has been transformed.
For some reason, line 236 (now commented out):
cost_fn = qml.map(circuit, self.observables, device=self.circuit.device, measure='expval')([],[])
throws the following error
It has something todo with fact that products of Paulis are considered tensors and not Observables.
I'm sure there is a better way of doing what I'm doing now, so I look forward to your thoughts @josh146 .