-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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 accumulate_grad_batches for last batch #2853
Fix accumulate_grad_batches for last batch #2853
Conversation
Hello @ydcjeff! Thanks for updating this PR.
Comment last updated at 2020-08-15 13:46:13 UTC |
Should I write new tests? |
yes, please, add a new test for this case... |
It is ready to review. |
This pull request is now in conflict... :( |
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
This pull request is now in conflict... :( |
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.
LGTM ✌️. Not sure about circleci BuildError
Try to merge master to get rid of the build error |
not sure if I merged it correctly but still the same BuildError. |
ummm... ok, we seem to have broken this haha. Can we go back to the commit without rebasing master? |
ok... i need to run but. Maybe the easiest thing is to start from master again and apply your changes using master as the base? i don't know why this is trying to pull changes from weeks ago |
@rohitgr7 can force push the branch with the local changes, then it should be back to how it was. |
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.
looking good now, this build config error must come from master.
not sure what just happened, but thanks for fixing @rohitgr7 |
awesome!
|
pep8 is not a problem here I think https://www.flake8rules.com/rules/W503.html |
@ydcjeff seems you do not run CirclCI tests on our side which is the reason why all TPU test are missing |
@Borda I fetched from upstream and worked on separate branch, not sure why TPU tests are missing. |
ok... i tested this on master and the tests do fail. let's merge so we can get rid of the circle ci thing and run tpu tests |
* first attempt * update changelog * fix pep8 and tests * Apply suggestions from code review Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> * added new tests * fixed tests * Apply suggestions from code review * used num_training_batches * fixed pep8 * fixed with is_last_batch suggested by @awaelchli * fixed with num_training_batches * fixed with num_training_batches * cleanup * fix test and update docs * fixed for alignment, update docs * minor changes * update doc Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com> Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
What does this PR do?
Fixes #1549
EDIT:
Issue to reproduce – Colab
https://colab.research.google.com/drive/1iMMKPMeghhfWD092lT3l9XKClY2YJYNQ?usp=sharing
Fixed issue to reproduce – Colab
https://colab.research.google.com/drive/1udmCrqYfJz2t8pEDi5pAIJKcyH_jHLLF?usp=sharing
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃