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

Issue #657 - Call on_train_end after early stopping #723

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

HarshSharma12
Copy link
Contributor

@HarshSharma12 HarshSharma12 commented Jan 21, 2020

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos, doc improvements)
  • Did you read the contributor guideline?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes Issue 657

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 🙃

@neggert
Copy link
Contributor

neggert commented Jan 21, 2020

Thanks for tackling this! This solution works, but I think a better solution would be to change the early return into a break. Then all the same code gets called regardless of whether the loop ended or we stopped early. Should be easier to maintain in the long run.

Could you also please add a test so we can make sure this problem doesn't pop up again in the future?

@williamFalcon
Copy link
Contributor

@HarshSharma12 rebase master please? it fixes GPU tests

@williamFalcon williamFalcon merged commit 432a0bc into Lightning-AI:master Jan 21, 2020
@HarshSharma12 HarshSharma12 mentioned this pull request Jan 21, 2020
4 tasks
@HarshSharma12
Copy link
Contributor Author

there were tests that already covered this but I updated them a little. created a new PR with the changes and test updates

@HarshSharma12 HarshSharma12 mentioned this pull request Jan 23, 2020
4 tasks
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