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

Document use case of passing test dataloaders to Trainer.test() #1992

Merged
merged 6 commits into from
May 29, 2020
Merged

Document use case of passing test dataloaders to Trainer.test() #1992

merged 6 commits into from
May 29, 2020

Conversation

authman
Copy link
Contributor

@authman authman commented May 28, 2020

Before submitting

What does this PR do?

Fixes #1990

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?

Always 🙃

@mergify mergify bot requested a review from a team May 28, 2020 18:00
@authman
Copy link
Contributor Author

authman commented May 28, 2020

Please note I based this off of docs/changelog as opposed to master because if I didn't, whoever merges it in would have had to deal with the merge conflicts. docs/changelog is currently ahead of master anyway, and this PR fits into that scope of work.

@Borda Borda added the docs Documentation related label May 28, 2020
@Borda Borda changed the title Issue 1990 Doc patch. Doc patch May 28, 2020
@mergify mergify bot requested a review from a team May 28, 2020 18:57
awaelchli
awaelchli reviewed May 28, 2020
View reviewed changes
docs/source/test_set.rst Outdated
Test additional data loaders
----------------------------
If no test sets were passed in to `trainer.fit()` or otherwise declared inside your
Copy link
Contributor

Choose a reason for hiding this comment

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

test sets can't be passed into trainer.fit
also, if they are declared in LightningModule, they get ignored and only the new dataloader(s) get used. I'm 99% sure.

Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/PyTorchLightning/pytorch-lightning/blob/8ee6d91d0e8ea83829a04d35e7fe7500eb332211/pytorch_lightning/trainer/trainer.py#L779-L784

Copy link
Member

Choose a reason for hiding this comment

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

@awaelchli is right here...

Copy link
Contributor

Choose a reason for hiding this comment

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

where? I only see val dataloaders

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my mistake. In my brain, I still had the code base for old version of PTL when test_dataloaders was indeed an argument passed into .fit: see here. But toady that is no longer the case.

Copy link
Member

Choose a reason for hiding this comment

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

I say that @awaelchli is correct, test set cannot be passed to .fit()

Copy link
Contributor

Choose a reason for hiding this comment

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

A slight reformulation would do it I think.

docs/source/test_set.rst Outdated
trainer.test(test_dataloaders=test)

You can either pass in either a single dataloader or a list of them. This
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
You can either pass in either a single dataloader or a list of them. This
You can pass in either a single dataloader or a list of them. This

@mergify mergify bot requested a review from a team May 28, 2020 18:59
@Borda Borda changed the base branch from docs/changelog to master May 28, 2020 19:00
@Borda Borda changed the base branch from master to docs/changelog May 28, 2020 19:02
@Borda
Copy link
Member

Borda commented May 28, 2020

@authman so we can close this PR... 🦝

@Borda Borda closed this May 28, 2020
@awaelchli
Copy link
Contributor

I think it would still be good to have the information to pass in dataloaders to Trainer.test

@Borda Borda reopened this May 28, 2020
@Borda
Copy link
Member

Borda commented May 28, 2020

@authman mind reformate it for test as @awaelchli suggested?

@authman
Copy link
Contributor Author

authman commented May 28, 2020

Working on the commit.

Borda
Borda approved these changes May 29, 2020
View reviewed changes
docs/source/test_set.rst Show resolved Hide resolved
docs/source/test_set.rst Outdated Show resolved Hide resolved
@Borda Borda added the ready PRs ready to be merged label May 29, 2020
@Borda Borda requested a review from awaelchli May 29, 2020 13:32
@mergify mergify bot requested a review from a team May 29, 2020 13:32
awaelchli
awaelchli approved these changes May 29, 2020
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.

great, thanks for your adjustments.

docs/source/test_set.rst Outdated Show resolved Hide resolved
docs/source/test_set.rst Outdated Show resolved Hide resolved
@mergify mergify bot requested a review from a team May 29, 2020 19:51
@awaelchli awaelchli linked an issue May 29, 2020 that may be closed by this pull request
Add Docs Demo'ing Test-Set Loaders With Trainer.Test() #1990
Closed
@awaelchli awaelchli changed the title Doc patch Document use case of passing test dataloaders to Trainer.test() May 29, 2020
@Borda Borda merged commit 2909011 into Lightning-AI:docs/changelog May 29, 2020
williamFalcon pushed a commit that referenced this pull request May 31, 2020
@Borda @authman @awaelchli
fix changelog (#1864)
9893681 
* fix chlog

* test for #1729

* hist

* update

* Document use case of passing test dataloaders to Trainer.test() (#1992)

* Issue 1990 Doc patch.

* Codeblock directive.

* Update to reflect current state of pytorch-lightning

* Final grammar cleaning. I hope these commits are squashed.

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Co-authored-by: authman <uapatira@gmail.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
justusschock pushed a commit that referenced this pull request Jun 29, 2020
@Borda @authman
@awaelchli @justusschock
fix changelog (#1864)
3937428 
* fix chlog

* test for #1729

* hist

* update

* Document use case of passing test dataloaders to Trainer.test() (#1992)

* Issue 1990 Doc patch.

* Codeblock directive.

* Update to reflect current state of pytorch-lightning

* Final grammar cleaning. I hope these commits are squashed.

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>

Co-authored-by: authman <uapatira@gmail.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda approved these changes

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
docs Documentation related ready PRs ready to be merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Docs Demo'ing Test-Set Loaders With Trainer.Test()
3 participants
@authman @Borda @awaelchli