Skip to content
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

Fix how trainable args are counted for gradients in GradientDescentOptimizer and NesterovMomentumOptimizer #1495

Merged
merged 25 commits into from
Aug 6, 2021
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
caed76a
count the trainable args, not simply the args
antalszava Aug 4, 2021
d586c24
Merge branch 'master' into ch7853-fix_grad_desc_mult_args
antalszava Aug 4, 2021
c79f5c0
changelog
antalszava Aug 4, 2021
b07357f
update nesterov opt too
antalszava Aug 4, 2021
08fa4eb
update nesterov opt too
antalszava Aug 4, 2021
c57a3fb
comment
antalszava Aug 4, 2021
4751c69
changelog update
antalszava Aug 4, 2021
9d2af7c
remove redundant enumerate
antalszava Aug 4, 2021
cbe3457
format
antalszava Aug 4, 2021
eba42af
add another test for two trainable args
antalszava Aug 4, 2021
0ea45d7
extract trainable args
antalszava Aug 4, 2021
c45226e
enumerate trainable args
antalszava Aug 4, 2021
d8ca734
Update tests/test_optimize.py
antalszava Aug 4, 2021
a0d468c
Update tests/test_optimize.py
antalszava Aug 4, 2021
386eb77
apply suggestion
antalszava Aug 4, 2021
e29725f
Merge branch 'ch7853-fix_grad_desc_mult_args' of github.com:PennyLane…
antalszava Aug 4, 2021
f448a4d
add new test case
antalszava Aug 4, 2021
210a32c
test docstring
antalszava Aug 4, 2021
0b58d3e
Revert "apply suggestion"
antalszava Aug 4, 2021
5e400fe
format
antalszava Aug 4, 2021
7a02ff8
remove extra dimensionality from non-trainable
antalszava Aug 4, 2021
2d85634
create a fixture that returns a new optimizer object for each test ca…
antalszava Aug 4, 2021
9e5526e
format test
antalszava Aug 4, 2021
675b7fc
Update tests/test_optimize.py
antalszava Aug 4, 2021
ad0eb38
Merge branch 'master' into ch7853-fix_grad_desc_mult_args
antalszava Aug 6, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,11 @@

<h3>Bug fixes</h3>

* Fixes a bug in `GradientDescentOptimizer` and `NesterovMomentumOptimizer`
where a cost function with one trainable parameter and non-trainable
parameters raised an error.
[(#1495)](https://github.com/PennyLaneAI/pennylane/pull/1495)

* Fixes a bug where the adjoint of `qml.QFT` when using the `qml.adjoint` function
was not correctly computed.
[(#1451)](https://github.com/PennyLaneAI/pennylane/pull/1451)
Expand Down
7 changes: 6 additions & 1 deletion pennylane/optimize/gradient_descent.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,12 @@ def compute_grad(objective_fn, args, kwargs, grad_fn=None):
grad = g(*args, **kwargs)
forward = getattr(g, "forward", None)

if len(args) == 1:
num_trainable_args = 0
for arg in args:
if getattr(arg, "requires_grad", True):
num_trainable_args += 1
Comment on lines +132 to +135
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antalszava you might be able to use the new

num_trainable_args = len(qml.math.get_trainable_indices(args))

functionality I just merged into the math module!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(optional, though)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This didn't seem to have worked for some tests 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries! We'll need to remember to update this in the future, if requires_grad=False by default

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue seems to be that if the argument is not a PennyLane NumPy tensor, then it'll be considered to belong to the "numpy" interface (instead of the "autgrad" interface), and all "numpy" arguments automatically evaluate as False when calling utils.requires_grad(arg). So, whenever an arg is simply a float get_trainable_indices would consider it non-trainiable, while that seems to not be what's wanted here.


if num_trainable_args == 1:
grad = (grad,)

return grad, forward
Expand Down
9 changes: 7 additions & 2 deletions pennylane/optimize/nesterov_momentum.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,13 @@ def compute_grad(self, objective_fn, args, kwargs, grad_fn=None):
"""
shifted_args = list(args)

trainable_args = []
for arg in args:
if getattr(arg, "requires_grad", True):
trainable_args.append(arg)

if self.accumulation:
for index, arg in enumerate(args):
for index, arg in enumerate(trainable_args):
if self.accumulation[index]:
x_flat = _flatten(arg)
acc = _flatten(self.accumulation[index])
Expand All @@ -82,7 +87,7 @@ def compute_grad(self, objective_fn, args, kwargs, grad_fn=None):
grad = g(*shifted_args, **kwargs)
forward = getattr(g, "forward", None)

if len(args) == 1:
if len(trainable_args) == 1:
grad = (grad,)

return grad, forward
114 changes: 106 additions & 8 deletions tests/test_optimize.py
Original file line number Diff line number Diff line change
Expand Up @@ -747,16 +747,41 @@ def reset(opt):
opt.reset()


@pytest.fixture
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to this structure, as it seems that the previous code left state in between test cases. This made 2 tests fail:

Before: https://github.com/PennyLaneAI/pennylane/runs/3244508688

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 💯

def opt(opt_name):
if opt_name == "gd":
return GradientDescentOptimizer(stepsize)

if opt_name == "nest":
return NesterovMomentumOptimizer(stepsize, momentum=gamma)

if opt_name == "moment":
return MomentumOptimizer(stepsize, momentum=gamma)

if opt_name == "ada":
return AdagradOptimizer(stepsize)

if opt_name == "rms":
return RMSPropOptimizer(stepsize, decay=gamma)

if opt_name == "adam":
return AdamOptimizer(stepsize, beta1=gamma, beta2=delta)

if opt_name == "roto":
return RotosolveOptimizer()


@pytest.mark.parametrize(
"opt, opt_name",
"opt_name",
[
(GradientDescentOptimizer(stepsize), "gd"),
(MomentumOptimizer(stepsize, momentum=gamma), "moment"),
(NesterovMomentumOptimizer(stepsize, momentum=gamma), "nest"),
(AdagradOptimizer(stepsize), "ada"),
(RMSPropOptimizer(stepsize, decay=gamma), "rms"),
(AdamOptimizer(stepsize, beta1=gamma, beta2=delta), "adam"),
(RotosolveOptimizer(), "roto"),
"gd",
"nest",
antalszava marked this conversation as resolved.
Show resolved Hide resolved
"moment",
"nest",
"ada",
"rms",
"adam",
"roto",
],
)
class TestOverOpts:
Expand Down Expand Up @@ -877,3 +902,76 @@ def func2(args):
assert x_seperate == pytest.approx(args_new[0], abs=tol)
assert y_seperate == pytest.approx(args_new[1], abs=tol)
assert z_seperate == pytest.approx(args_new[2], abs=tol)

def test_one_trainable_one_non_trainable(self, opt, opt_name, tol):
"""Tests that a cost function that takes one trainable and one
non-trainable parameter executes well."""
dev = qml.device("default.qubit", wires=2)

@qml.qnode(dev)
def circuit(x):
qml.RX(x, wires=0)
return qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))

def cost(x, target):
return (circuit(x) - target) ** 2

ev = np.tensor(0.7781, requires_grad=False)
x = np.tensor(0.0, requires_grad=True)

original_ev = ev

(x, ev), cost = opt.step_and_cost(cost, x, ev)
antalszava marked this conversation as resolved.
Show resolved Hide resolved

# check that the argument to RX doesn't change, as the X rotation doesn't influence <Z>
assert x == 0
assert ev == original_ev

def test_one_non_trainable_one_trainable(self, opt, opt_name, tol):
"""Tests that a cost function that takes one non-trainable and one
trainable parameter executes well."""
dev = qml.device("default.qubit", wires=2)

@qml.qnode(dev)
def circuit(x):
qml.RX(x, wires=0)
return qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))

def cost(target, x): # Note: the order of the arguments has been swapped
return (circuit(x) - target) ** 2

ev = np.tensor(0.7781, requires_grad=False)
x = np.tensor(0.0, requires_grad=True)

original_ev = ev

(ev, x), cost = opt.step_and_cost(cost, ev, x)
# check that the argument to RX doesn't change, as the X rotation doesn't influence <Z>
assert x == 0
assert ev == original_ev

def test_two_trainable_args(self, opt, opt_name, tol):
"""Tests that a cost function that takes at least two trainable
arguments executes well."""
dev = qml.device("default.qubit", wires=2)

@qml.qnode(dev)
def circuit(x, y):
qml.RX(x, wires=0)
qml.RX(y, wires=0)
return qml.expval(qml.PauliZ(0) @ qml.PauliZ(1))

def cost(x, y, target):
return (circuit(x, y) - target) ** 2

ev = np.tensor(0.7781, requires_grad=False)
x = np.tensor(0.0, requires_grad=True)
y = np.tensor(0.0, requires_grad=True)

original_ev = ev

(x, y, ev), cost = opt.step_and_cost(cost, x, y, ev)

# check that the argument to RX doesn't change, as the X rotation doesn't influence <Z>
assert x == 0
assert ev == original_ev