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

Update examples - use DataModule #4740

Merged
merged 11 commits into from
Nov 20, 2020
Merged

Update examples - use DataModule #4740

merged 11 commits into from
Nov 20, 2020

Conversation

Borda
Copy link
Member

@Borda Borda commented Nov 18, 2020 •
edited
Loading

What does this PR do?

partial update for #4694

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?

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In in short, see following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified; Bugfixes should be including in bug-fix release milestones (m.f.X) and features should be included in (m.X.b) releases.

Did you have fun?

Make sure you had fun coding 🙃

@Borda Borda added feature Is an improvement or enhancement example labels Nov 18, 2020
@Borda Borda added this to the 1.1 milestone Nov 18, 2020
@pep8speaks
Copy link

pep8speaks commented Nov 18, 2020 •
edited
Loading

Hello @Borda! Thanks for updating this PR.

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

Comment last updated at 2020-11-20 15:25:50 UTC

@Borda Borda requested review from nateraw and awaelchli and removed request for nateraw November 18, 2020 14:41
@Borda Borda marked this pull request as ready for review November 18, 2020 21:58
@codecov
Copy link

codecov bot commented Nov 18, 2020 •
edited
Loading

Codecov Report

Merging #4740 (519e29f) into master (42e59c6) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4740   +/-   ##
======================================
  Coverage      93%     93%           
======================================
  Files         117     117           
  Lines        8967    8972    +5     
======================================
+ Hits         8335    8345   +10     
+ Misses        632     627    -5

awaelchli
awaelchli reviewed Nov 19, 2020
View reviewed changes
pl_examples/__init__.py Outdated

EXAMPLES_ROOT = os.path.dirname(__file__)
PACKAGE_ROOT = os.path.dirname(EXAMPLES_ROOT)
DATASETS_PATH = os.path.join(PACKAGE_ROOT, 'Datasets')
Copy link
Contributor

@awaelchli awaelchli Nov 18, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

maybe "datasets" lowercase?

Copy link
Member Author

Choose a reason for hiding this comment

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

mean the first or second one?

pytorch_lightning/utilities/__init__.py Show resolved Hide resolved
tchaton
tchaton approved these changes Nov 19, 2020
View reviewed changes
Copy link
Contributor

@tchaton tchaton left a comment

Choose a reason for hiding this comment

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

Great PR ! Let's use _module_available for all third party libraries.
Would you mind adding a test for _module_available too ?

pytorch_lightning/utilities/__init__.py Show resolved Hide resolved
pl_examples/basic_examples/backbone_image_classifier.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member Author

Borda commented Nov 19, 2020

Great PR ! Let's use _module_available for all third party libraries.
Would you mind adding a test for _module_available too ?

it is already tested by its example - doctest

@Borda Borda requested a review from awaelchli November 19, 2020 09:19
@Borda Borda added the ready PRs ready to be merged label Nov 19, 2020
@Borda Borda requested a review from SkafteNicki November 20, 2020 14:36
SeanNaren
SeanNaren reviewed Nov 20, 2020
View reviewed changes
pl_examples/basic_examples/dali_image_classifier.py
from nvidia.dali.pipeline import Pipeline
from nvidia.dali.plugin.pytorch import DALIClassificationIterator
except (ImportError, ModuleNotFoundError):
else:
warn('NVIDIA DALI is not available')
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this isn't related to this PR, but shouldn't this be a hard crash if DALI isn't available?

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, the case is that with tests running on CPU where we do not install DALI the testing will fail just because of this raising error, an alternative would be to return from this script if DALI is missing without crashing...

nateraw
nateraw approved these changes Nov 20, 2020
View reviewed changes
Copy link
Contributor

@nateraw nateraw left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@rohitgr7 rohitgr7 merged commit 94a9d3d into master Nov 20, 2020
@rohitgr7 rohitgr7 deleted the update-examples branch November 20, 2020 18:10
rohitgr7 pushed a commit that referenced this pull request Nov 21, 2020
@Borda @rohitgr7
Update examples - use DataModule (#4740)
7fd9aa2 
* rename

* add mnist_datamodule.py

* dm

* fix

* imports

* clean

* imports

* transforms

* skip
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nateraw nateraw nateraw approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@tchaton tchaton tchaton approved these changes

@SeanNaren SeanNaren SeanNaren approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

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

@teddykoker teddykoker Awaiting requested review from teddykoker

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

@awaelchli awaelchli Awaiting requested review from awaelchli

@SkafteNicki SkafteNicki Awaiting requested review from SkafteNicki

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

Successfully merging this pull request may close these issues.
8 participants
@Borda @pep8speaks @awaelchli @SeanNaren @tchaton @SkafteNicki @rohitgr7 @nateraw