-
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
Fix how trainable args are counted for gradients in GradientDescentOptimizer
and NesterovMomentumOptimizer
#1495
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1495 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 183 183
Lines 13163 13171 +8
=======================================
+ Hits 12948 12956 +8
Misses 215 215
Continue to review full report at Codecov.
|
GradientDescentOptimizer
and NesterovMomentumOptimizer
num_trainable_args = 0 | ||
for arg in args: | ||
if getattr(arg, "requires_grad", True): | ||
num_trainable_args += 1 |
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.
@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!
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.
(optional, though)
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 didn't seem to have worked for some tests 🤔
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.
No worries! We'll need to remember to update this in the future, if requires_grad=False
by default
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.
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.
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.
Really nice catch @antalszava 💯
I've left some minor comments - only important one is to add one additional test case.
num_trainable_args = 0 | ||
for arg in args: | ||
if getattr(arg, "requires_grad", True): | ||
num_trainable_args += 1 |
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.
(optional, though)
Co-authored-by: Josh Izaac <josh146@gmail.com>
Co-authored-by: Josh Izaac <josh146@gmail.com>
…AI/pennylane into ch7853-fix_grad_desc_mult_args
This reverts commit 386eb77.
…se; the previous version left a state because the object lived on
@@ -747,16 +747,41 @@ def reset(opt): | |||
opt.reset() | |||
|
|||
|
|||
@pytest.fixture |
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.
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
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! 💯
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 @antalszava 👨🍳
num_trainable_args = 0 | ||
for arg in args: | ||
if getattr(arg, "requires_grad", True): | ||
num_trainable_args += 1 |
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.
No worries! We'll need to remember to update this in the future, if requires_grad=False
by default
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 fix @antalszava!
@@ -747,16 +747,41 @@ def reset(opt): | |||
opt.reset() | |||
|
|||
|
|||
@pytest.fixture |
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! 💯
num_trainable_args = 0 | ||
for arg in args: | ||
if getattr(arg, "requires_grad", True): | ||
num_trainable_args += 1 |
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.
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.
Context
The
GradientDescentOptimizer
andNesterovMomentumOptimizer
optimizers seem to have an issue with a cost function that takes one trainable and one non-trainable argument:Raises
Although the cost function does not depend on
target
, the error is still raised. If we remove that argument, the gradient is computed well:For both optimizers, there is a part of the logic, where the output gradient is adjusted:
Changes made
Changes how the output gradient is adjusted such that only the trainable arguments are considered.
Related issues
PennyLaneAI/qml#309