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 problem with sparse layers in tf0.1.0 #20

Merged
merged 19 commits into from
Nov 13, 2020

Conversation

cryu854
Copy link
Contributor

@cryu854 cryu854 commented Oct 29, 2020

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.

@juntang-zhuang
Copy link
Owner

@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: tensorflow/addons#2203, if you are interested we can create a pull request and upload Adabelief code based on current version.

@cryu854
Copy link
Contributor Author

cryu854 commented Nov 4, 2020

@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.

@juntang-zhuang
Copy link
Owner

Thanks so much for help!

@cryu854
Copy link
Contributor Author

cryu854 commented Nov 7, 2020

@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.

@juntang-zhuang
Copy link
Owner

@cryu854 Nice work! Thanks so much for help. I quickly tested it and it works quite good.

@cryu854
Copy link
Contributor Author

cryu854 commented Nov 9, 2020

@juntang-zhuang I've made this PR #2234 to addons. Thank you for giving me this opportunity to contribute to tensorflow-addons.

@cryu854
Copy link
Contributor Author

cryu854 commented Nov 13, 2020

@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:

  • Since tensorflow 2 uses Eager mode and AutoGraph, there is no need to ensure the assignment dependencies within the context manager tf.control_dependencies.
  • Removed register and addons dependencies.
  • Added unit tests for AdaBelief.py.
  • Fixed some typos at README.md and setup.py.

Hope these help!

@juntang-zhuang juntang-zhuang merged commit 100e8c3 into juntang-zhuang:update_0.1.0 Nov 13, 2020
@juntang-zhuang
Copy link
Owner

@cryu854 Thanks so much, I just merged your pull request. Thanks gain for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants