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

Also update progress_bar in training_epoch_end #1724

Merged
merged 7 commits into from
May 9, 2020
Merged

Also update progress_bar in training_epoch_end #1724

merged 7 commits into from
May 9, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented May 4, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1448.

I noticed we don't have thorough testing for the LightningModule methods. We should test that the outputs of these methods and hooks get correctly passed to the trainer. Therefore I added a module subdirectory. One file per method, i.e., training_step, training_epoch_end, ...
What do you think?

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 🙃

@awaelchli awaelchli added the feature Is an improvement or enhancement label May 4, 2020
@mergify mergify bot requested a review from a team May 4, 2020 01:25
@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #1724 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1724   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          69      69           
  Lines        4156    4157    +1     
======================================
+ Hits         3666    3668    +2     
+ Misses        490     489    -1     

@pep8speaks
Copy link

pep8speaks commented May 4, 2020

Hello @awaelchli! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-05-07 21:49:09 UTC

@awaelchli awaelchli changed the title [WIP] Also update progress_bar in training_epoch_end Also update progress_bar in training_epoch_end May 6, 2020
@awaelchli awaelchli marked this pull request as ready for review May 6, 2020 02:11
@awaelchli awaelchli added the ci Continuous Integration label May 6, 2020
@mergify mergify bot requested a review from a team May 7, 2020 22:07
Copy link
Contributor

@jeremyjordan jeremyjordan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always good to add better test coverage :)

@mergify mergify bot requested a review from a team May 8, 2020 00:58
@williamFalcon williamFalcon merged commit 25bbd05 into Lightning-AI:master May 9, 2020
@awaelchli awaelchli deleted the feature/pbar_metrics_epoch branch May 9, 2020 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci Continuous Integration feature Is an improvement or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Also update progress_bar in training_epoch_end
5 participants