Add gradient ascent optimization algorithms #247
Conversation
This is part of the effort to port the work by Krzysztof Choromanski, Mark Rowland, Vikas Sindhwani, Richard E. Turner, Adrian Weller: "Structured Evolution with Compact Architectures for Scalable Policy Optimization", https://arxiv.org/abs/1804.02395
|
|
||
|
|
||
| # TODO(kchoro): Borrow JAXs optimizer library here. Integrated into Blackbox-v2. | ||
| class GAOptimizer(metaclass=abc.ABCMeta): |
There was a problem hiding this comment.
Can you follow the style guide a bit more closely?
- Avoid acronyms in names. GAOptimizer --> GradientAscentOptimizer or similar
- Can you use appropriate pytypes in all signatures and make sure this code passes pytpye checks?
There was a problem hiding this comment.
The rename will be addressed after porting and is kept track of with Issue #254.
The signatures will be addressed in a type annotations PR after the initial port.
| ############################################################################### | ||
| r"""Library of gradient ascent algorithms. | ||
|
|
||
| Library of stateful gradient descent algorithms taking as input the gradient and |
There was a problem hiding this comment.
Perhaps it's worth calling the class GradientDescent and passing in the negative instead? Or change this text to gradient ascent?
There was a problem hiding this comment.
The text has been changed to gradient ascent in the related commit
|
|
||
|
|
||
| class AdamOptimizer(GAOptimizer): | ||
| """Class implementing ADAM gradient ascent optimizer.""" |
There was a problem hiding this comment.
Please describe the state vector here and in other optimizers' docstrings. Looks like here it's [mmv1] + [mmv2] + [1] where the last dim corresponds to the current step?
There was a problem hiding this comment.
Descriptions for the states have been added in the related commit
| 0.1, 0.9) | ||
| optimizer.set_state(state) | ||
| recovered_state = optimizer.get_state() | ||
| np.testing.assert_array_equal(state, recovered_state) |
There was a problem hiding this comment.
avoid using equal for floats, even if you expect input and output to be identical. and certainly do not use equal when FP calculations are involved, like below.
There was a problem hiding this comment.
prefer np.testing.assert_close/near/approx and similar.
There was a problem hiding this comment.
Please remember to fix this in the next PR.
There was a problem hiding this comment.
The instances have been changed to assert_array_almost_equal in the related commit
The plan has been detailed in Issue #248 |
This is part of the effort to port the work by Krzysztof Choromanski, Mark Rowland, Vikas Sindhwani, Richard E. Turner, Adrian Weller: "Structured Evolution with Compact Architectures for Scalable Policy Optimization", https://arxiv.org/abs/1804.02395
First step in Issue #248