-
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
Simplify progress bar args #1108
Simplify progress bar args #1108
Conversation
Hello @gerardrbentley! Thanks for updating this PR.
Comment last updated at 2020-04-02 22:32:08 UTC |
hey there, we have added GPU CI test, so could we kindly ask to rebase/merge master which will trigger these tests so we do not need to test it manually... Thx for your understanding 🤖 |
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.
Thx for taking care of this change, here are some todos to be completed:
- update changelog
- could we kindly ask to rebase/merge
master
Adding your suggestions right now |
0b84ec0
to
ffe7595
Compare
Forget to add a commit for Changelog, @Borda should I add that on this PR or is there a different way the changelog is updated? (Sorry, new to contributing) EDIT |
It is kind of responsibility to edit the changelog along with the change in a particular PR |
This pull request is now in conflict... :( |
Codecov Report
@@ Coverage Diff @@
## master #1108 +/- ##
========================================
Coverage ? 91%
========================================
Files ? 61
Lines ? 3153
Branches ? 0
========================================
Hits ? 2880
Misses ? 273
Partials ? 0 |
@gerardrbentley how is it going? could you please rebase on master so it is clear what are the changes... |
9fc6f82
to
bedb285
Compare
@Borda Hopefully done. Rebased and I think I cut down all the erroneous things I tried today in my git log. Apologies, definitely made some errors around the flake8 checking and |
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.
pls move the deprecated attributes to deprecated API
This pull request is now in conflict... :( |
db9dcdf
to
be80603
Compare
This pull request is now in conflict... :( |
This pull request is now in conflict... :( |
Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>
some issues with GitHub, need to accept commits... |
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.
Trying to Commit Suggestions
, but it's telling me the diff is outdated
Co-Authored-By: Jirka Borovec <Borda@users.noreply.github.com>
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 🚀
I think I might have missed that /test_amp.py fix. Bulk commit suggestions didn't work and only one went through I believe |
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 :)
there was as incident recently... https://www.githubstatus.com/ |
* show progress bar dependent on refresh_rate * test progress_bar_refresh control show bar * remove show_progress_bar from other tests * borda fixes * flake8 fix * changelog update prog bar refresh rate * move show_progress_bar to deprecated 0.9 api * rm show_progress_bar references, test deprecated * Update pytorch_lightning/trainer/__init__.py * fix test * changelog * minor CHANGELOG.md format * Update pytorch_lightning/trainer/__init__.py * Update pytorch_lightning/trainer/trainer.py Co-authored-by: Gerard Bentley <gbkh2015@mymail.pomona.edu> Co-authored-by: William Falcon <waf2107@columbia.edu> Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com> Co-authored-by: J. Borovec <jirka.borovec@seznam.cz>
Before submitting
What does this PR do?
Fixes #1102 suggestion to collapse
show_progress_bar
intoprogress_bar_refresh_rate
Adds show_progress_bar to "to be deprecated" Trainer args (unsure on best approach here for community)
Removes show_progress_bar from tests that used it
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 🙃
Learned some Git 🙃