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 test configuration check and testing #1804

Merged
merged 6 commits into from
May 17, 2020
Merged

Fix test configuration check and testing #1804

merged 6 commits into from
May 17, 2020

Conversation

rohitgr7
Copy link
Contributor

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 #1720 and #1754.

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 🙃

@pep8speaks
Copy link

pep8speaks commented May 12, 2020

Hello @rohitgr7! Thanks for updating this PR.

Line 1096:111: E501 line too long (119 > 110 characters)

Comment last updated at 2020-05-13 13:11:07 UTC

@mergify
Copy link
Contributor

mergify bot commented May 12, 2020

This pull request is now in conflict... :(

@mergify mergify bot requested a review from a team May 12, 2020 19:05
@rohitgr7
Copy link
Contributor Author

Working on failed test cases.

raise MisconfigurationException('You have passed in a `val_dataloader()`'
' but have not defined `validation_step()`.')
if not self.testing:
if not self.is_overridden('training_step', model):
Copy link
Member

Choose a reason for hiding this comment

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

I would consider moving this to __check_model_configuration_test and alternatively bellow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest removing check_testing_model_configuration because it's just a repeated code and testing configuration can be handled within check_model_configuration. Also during testing check_model_configuration is called which is unnecessary and was not working in #1720. The PR fixes both the issues.

@mergify mergify bot requested a review from a team May 13, 2020 13:20
@codecov
Copy link

codecov bot commented May 13, 2020

Codecov Report

Merging #1804 into master will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1804   +/-   ##
======================================
- Coverage      88%     88%   -0%     
======================================
  Files          71      71           
  Lines        4431    4422    -9     
======================================
- Hits         3893    3885    -8     
+ Misses        538     537    -1     

@williamFalcon williamFalcon added this to the 0.7.7 milestone May 13, 2020
@rohitgr7 rohitgr7 requested review from Borda and removed request for a team May 13, 2020 18:27
@mergify mergify bot requested a review from a team May 13, 2020 18:27
@awaelchli awaelchli mentioned this pull request May 16, 2020
5 tasks
@williamFalcon williamFalcon merged commit 56d521a into Lightning-AI:master May 17, 2020
@williamFalcon
Copy link
Contributor

Thank you!

@rohitgr7 rohitgr7 deleted the fix_test branch May 17, 2020 12:37
@Borda Borda modified the milestones: 0.7.7, 0.8.0 May 26, 2020
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.

Incompatible Trainer.test()
5 participants