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.

Sign up for GitHub

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

Jump to bottom

Throw MisconfigurationError on unknown mode #5255

Merged
merged 7 commits into from
Jan 12, 2021
Merged

Throw MisconfigurationError on unknown mode #5255

merged 7 commits into from
Jan 12, 2021

Conversation

alanhdu
Copy link
Contributor

@alanhdu alanhdu commented Dec 23, 2020 •
edited
Loading

Potential closes #5252

When an unknown mode is passed to EarlyStopping or ModelCheckpoitn, raise a misconfiguration error instead of just a runtime warning.

@alanhdu alanhdu mentioned this pull request Dec 23, 2020
Closed
@codecov
Copy link

codecov bot commented Dec 23, 2020 •
edited
Loading

Codecov Report

Merging #5255 (cd0ee01) into release/1.2-dev (059f463) will increase coverage by 0%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##           release/1.2-dev   #5255   +/-   ##
===============================================
  Coverage               93%     93%           
===============================================
  Files                  151     151           
  Lines                10621   10621           
===============================================
+ Hits                  9836    9839    +3     
+ Misses                 785     782    -3

ydcjeff
ydcjeff reviewed Dec 25, 2020
View reviewed changes
Copy link
Contributor

@ydcjeff ydcjeff left a comment

Choose a reason for hiding this comment

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

Would you mind add a test for that?

@pep8speaks
Copy link

pep8speaks commented Jan 4, 2021 •
edited
Loading

Hello @alanhdu! Thanks for updating this PR.

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

Comment last updated at 2021-01-12 06:52:38 UTC

@alanhdu
Copy link
Contributor Author

alanhdu commented Jan 4, 2021

@ydcjeff: Done (sorry for the delay -- I was out during the holidays)!

Rebased onto the latest master as well.

awaelchli
awaelchli approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@awaelchli awaelchli left a comment

Choose a reason for hiding this comment

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

nice.
a minor improvement would be to print available modes in the misconfiguration exception, i.e.,
mode x is unknown. Available modes are min, max.

@awaelchli awaelchli added the feature Is an improvement or enhancement label Jan 5, 2021
@awaelchli awaelchli added this to the 1.2 milestone Jan 5, 2021
ydcjeff
ydcjeff reviewed Jan 5, 2021
View reviewed changes
Copy link
Contributor

@ydcjeff ydcjeff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

these tests can be removed?
pls ignore

ydcjeff
ydcjeff reviewed Jan 5, 2021
View reviewed changes
Copy link
Contributor

@ydcjeff ydcjeff left a comment

Choose a reason for hiding this comment

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

These tests can be removed not to collide with deprecated ones?

tests/callbacks/test_early_stopping.py Outdated Show resolved Hide resolved
tests/callbacks/test_early_stopping.py Outdated Show resolved Hide resolved
@Borda Borda changed the base branch from master to release/1.2-dev January 5, 2021 17:12
@Borda
Copy link
Member

Borda commented Jan 5, 2021 •
edited
Loading

@alanhdu fyi, I have rebased it on PyTorchLightning:release/1.2-dev 🐰

@Borda Borda enabled auto-merge (squash) January 5, 2021 17:18
rohitgr7
rohitgr7 reviewed Jan 6, 2021
View reviewed changes
tests/callbacks/test_early_stopping.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/early_stopping.py Outdated Show resolved Hide resolved
pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved
tests/callbacks/test_early_stopping.py Outdated Show resolved Hide resolved
tests/callbacks/test_early_stopping.py Outdated Show resolved Hide resolved
rohitgr7
rohitgr7 approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@rohitgr7 rohitgr7 left a comment

Choose a reason for hiding this comment

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

nice!

@alanhdu
Copy link
Contributor Author

alanhdu commented Jan 8, 2021

Rebased to fix the merge conflict.

@awaelchli awaelchli added the ready PRs ready to be merged label Jan 9, 2021
@Borda Borda merged commit f6dc354 into Lightning-AI:release/1.2-dev Jan 12, 2021
@alanhdu alanhdu deleted the alan/error branch January 12, 2021 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ydcjeff ydcjeff ydcjeff left review comments

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@justusschock justusschock justusschock approved these changes

@Borda Borda Borda approved these changes

@awaelchli awaelchli awaelchli approved these changes

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@edenlightning edenlightning Awaiting requested review from edenlightning

@teddykoker teddykoker Awaiting requested review from teddykoker

Assignees
No one assigned
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.2
Development

Successfully merging this pull request may close these issues.

Make EarlyStopping throw an error on unknown modes
8 participants
@alanhdu @pep8speaks @Borda @awaelchli @justusschock @rohitgr7 @ydcjeff @tchaton