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

Fix trivial comparison in model checkpoint test #3464

Merged
merged 1 commit into from
Sep 11, 2020
Merged

Fix trivial comparison in model checkpoint test #3464

merged 1 commit into from
Sep 11, 2020

Conversation

ananthsub
Copy link
Contributor

@ananthsub ananthsub commented Sep 11, 2020

What does this PR do?

The model checkpoint test was keys across the same checkpoint dict instead of ckpt_last vs ckpt_last_epoch
This PR fixes that. All other changes here are formatting

Fixes # (issue)
Follows up on discussion in #2501

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests?
  • Did you verify new and existing tests pass locally with your changes?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

We were comparing keys across the same checkpoint dict instead of ckpt_last vs ckpt_last_epoch

All other changes here are formatting
@mergify mergify bot requested a review from a team September 11, 2020 16:15
for key in checkpoint_callback_keys:
assert (
ckpt_last_epoch["callbacks"][type(model_checkpoint)][key]
ckpt_last["callbacks"][type(model_checkpoint)][key]
Copy link
Contributor Author

@ananthsub ananthsub Sep 11, 2020

Choose a reason for hiding this comment

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

this is the only logic change. all other changes are from auto-formatting

@Borda Borda added the bug Something isn't working label Sep 11, 2020
@mergify mergify bot requested a review from a team September 11, 2020 17:32
@mergify mergify bot requested a review from a team September 11, 2020 17:42
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.

Thanks @ananthsub

@codecov
Copy link

codecov bot commented Sep 11, 2020

Codecov Report

Merging #3464 into master will increase coverage by 5%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #3464    +/-   ##
=======================================
+ Coverage      85%     90%    +5%     
=======================================
  Files         105     105            
  Lines        8157    8157            
=======================================
+ Hits         6943    7327   +384     
+ Misses       1214     830   -384     

@awaelchli awaelchli merged commit d1d48e2 into Lightning-AI:master Sep 11, 2020
@Borda Borda added this to the 0.9.x milestone Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants