Skip to content

Add missing test for "multiple dataloader + percent_check fix" #2226

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
Merged
merged 31 commits into from
Jun 23, 2020
Merged

Add missing test for "multiple dataloader + percent_check fix" #2226

merged 31 commits into from
Jun 23, 2020

Conversation

@awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Jun 17, 2020

What does this PR do?

This adds a missing test which got lost when #1920 was taken over by #2213 .
Hope it's ok with you @williamFalcon ?

@awaelchli awaelchli added the bug Something isn't working label Jun 17, 2020
@mergify mergify bot requested a review from a team June 17, 2020 21:09
@awaelchli awaelchli linked an issue Jun 17, 2020 that may be closed by this pull request
Incorrect number of batches when multiple test loaders are used and test_percent_check is specified #1899
Closed
@awaelchli
Copy link
Contributor Author

awaelchli commented Jun 17, 2020

this is rebased, and the tests that fail come from master.

@Borda
Copy link
Collaborator

Borda commented Jun 17, 2020

docs shall be fixed in #2227

@awaelchli awaelchli mentioned this pull request Jun 17, 2020
Merged
5 tasks
Borda
Borda requested changes Jun 17, 2020
View reviewed changes
Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

pls rebase on master

@mergify mergify bot requested a review from a team June 17, 2020 21:53
rohitgr7
rohitgr7 reviewed Jun 17, 2020
View reviewed changes
@rohitgr7
Copy link
Contributor

rohitgr7 commented Jun 17, 2020

Also some doc updates here:
limit_train_batches: How much of training dataset to check (floats = percent, int = num_batches)

https://github.com/PyTorchLightning/pytorch-lightning/blob/1635ba1bb38e69d985b9a96e1e8f6b6dbdeff268/pytorch_lightning/trainer/trainer.py#L226-L228

@codecov
Copy link

codecov bot commented Jun 17, 2020 •
edited
Loading

Codecov Report

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

@@          Coverage Diff           @@
##           master   #2226   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          70      70           
  Lines        5501    5500    -1     
======================================
+ Hits         4834    4835    +1     
+ Misses        667     665    -2

@rohitgr7 @Borda @awaelchli
Fix num batches in case of multiple dataloaders and percent_check (#1920
4cc7223 
)

* git conflict

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: Adrian Wälchli <[email protected]>
@Borda Borda added the ci Continuous Integration label Jun 18, 2020
@Borda Borda added this to the 0.8.0 milestone Jun 18, 2020
@Borda
Copy link
Collaborator

Borda commented Jun 18, 2020

@awaelchli @rohitgr7 I have merged the #1920 here to preserve the contribution of both, mind check and fix the tests, pls 🐰

@mergify
Copy link
Contributor

mergify bot commented Jun 18, 2020

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

@Borda Borda modified the milestones: 0.8.0, 0.8.x Jun 18, 2020
@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2020

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

@mergify
Copy link
Contributor

mergify bot commented Jun 20, 2020

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

Borda
Borda approved these changes Jun 21, 2020
View reviewed changes
Copy link
Collaborator

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🤖

@mergify mergify bot requested a review from a team June 21, 2020 20:11
@Borda Borda added the ready PRs ready to be merged label Jun 21, 2020
SkafteNicki
SkafteNicki approved these changes Jun 23, 2020
View reviewed changes
Copy link
Collaborator

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

LGTM

@mergify mergify bot requested a review from a team June 23, 2020 13:26
@williamFalcon williamFalcon merged commit e085e93 into master Jun 23, 2020
@Borda Borda deleted the num_batches_missing_test branch June 23, 2020 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Borda Borda Borda approved these changes

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@tullie tullie Awaiting requested review from tullie

@jeremyjordan jeremyjordan Awaiting requested review from jeremyjordan

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

+1 more reviewer

@rohitgr7 rohitgr7 rohitgr7 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working ci Continuous Integration ready PRs ready to be merged

Projects

None yet

Milestone

pl:0.8.x

Development

Successfully merging this pull request may close these issues.

Incorrect number of batches when multiple test loaders are used and test_percent_check is specified

5 participants

@awaelchli @Borda @rohitgr7 @SkafteNicki @williamFalcon