Closed cryu854 closed 3 years ago
@cryu854 Hi, just curious to check if you are still interested in contributing to the tensorflow-addons project? I think you are much better than me for this task because you have much more experience with Tensorlfow. Please see a feature request under tensorflow-addons project: https://github.com/tensorflow/addons/issues/2203, if you are interested we can create a pull request and upload Adabelief code based on current version.
@juntang-zhuang Yes, I would like to help with this. Here is a tensorflow-addons's CONTRIBUTING.md doc, explaining the review process and how to write tests for the contribution. In order to run the tests, we may also need to set up the development environment too. I'll look into the details of some tensorflow-addons projects and let you know when I have any progress.
Thanks so much for help!
@juntang-zhuang Hi, I've implemented the test code for AdaBelief, the code adabelief_test.py
is a combination of lamb_test.py
and rectified_adam_test.py
. Please take a look at my fork of addons, I will take any suggestion or modification if anything else is needed. And if all goes well, we can make a PR to addons to conduct further code reviews.
@cryu854 Nice work! Thanks so much for help. I quickly tested it and it works quite good.
@juntang-zhuang I've made this PR #2234 to addons. Thank you for giving me this opportunity to contribute to tensorflow-addons.
@juntang-zhuang Hi, I think the PR for addons will be postponed for a while, so I did the following modifications for current tensorflow version:
tf.control_dependencies
.AdaBelief.py
.README.md
and setup.py
.Hope these help!
@cryu854 Thanks so much, I just merged your pull request. Thanks gain for your contribution.
The
_resource_apply_sparse
function applies update according to the indices. Gathering the elements before update would fix the error caused by ResourceScatterAdd. The fix has been tested on word embeddings.