use accumulation for adam #1757
Conversation
|
Hey @albi3ro, I opened this WIP PR so you can maybe have a look whether the changes are along the lines of what you suggested. Cheers Moritz |
…ne into adam-accumulation
MoritzWillmann
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1757 +/- ##
=======================================
Coverage 99.22% 99.22%
=======================================
Files 209 209
Lines 15617 15626 +9
=======================================
+ Hits 15496 15505 +9
Misses 121 121
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks so much @MoritzWillmann 🎉 😃
I have a couple minor requests for changes, but the essence is all there.
pennylane/optimize/adam.py
Outdated
| self.t += 1 | ||
|
|
||
| if self.accumulation is None: | ||
| accumulation = namedtuple("accumulation", "fm sm t") |
There was a problem hiding this comment.
We could probably move this definition outside the class, around Line 22 or so. That way we won't be redefining the type a bunch of times.
There was a problem hiding this comment.
Okay, I'll change that
pennylane/optimize/adam.py
Outdated
|
|
||
| if self.accumulation is None: | ||
| accumulation = namedtuple("accumulation", "fm sm t") | ||
| self.accumulation = accumulation([None] * len(args), [None] * len(args), [0]) |
There was a problem hiding this comment.
| self.accumulation = accumulation([None] * len(args), [None] * len(args), [0]) | |
| self.accumulation = accumulation([None] * len(args), [None] * len(args), 0) |
We only need the latest time, so we don't need to store it in an array.
This change would need to get propagated throughout the file, changing self.accumulation.t[0] to self.accumulation.t.
There was a problem hiding this comment.
Actually, I think t needs to be defined mutable. Subclasses of namedtuples can't be changed naturally. Alternatively I could use a class definition or a dictionary.
There was a problem hiding this comment.
Good point. I forgot about that...
Maybe we should then just use a dictionary. Even though we technically could just keep using [0] wrapped in a list, it's probably better to use an intrinsically mutable type instead.
There was a problem hiding this comment.
Okay, I'll change it to a dictionary 🙂
Co-authored-by: Christina Lee <chrissie.c.l@gmail.com>
Co-authored-by: Christina Lee <chrissie.c.l@gmail.com>
tests/optimize/test_optimize.py
Outdated
| """Test the newly added property interfaces""" | ||
| bunch.adam_opt.reset() | ||
| # check if errors get raised when accumulation is empty | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
Remember to update the tests with the new return behaviour
There was a problem hiding this comment.
Thanks for the reminder!
|
Once the branch is up to date with master I'll merge it in. |
|
@albi3ro Cool, thanks! 🥳 |
This PR will introduce the usage of
accumulationto the Adam optimizer.Context: All other optimisers use a single class variable called
accumulationto keep track of running quantities while Adam uses three:fm,smandt. This should be changed to standardizeaccumulationacross all optimizers.Description of the Change: Use named tuple
accumulationincluding the previous variablesfm,smandt.Benefits: Standardization
Possible Drawbacks: Not Sure
Related GitHub Issues: closes #1580