Refactor Vision DataModules #400

chris-clem · 2020-11-24T13:25:48Z

What does this PR do?

As proposed in #395, this PR creates a BaseDataModule, that can be used as the parent class for several existing DataModules like BinaryMNISTDataModule, CIFAR10DataModule, FashionMNISTDataModule, and MNISTDataModule.

This removes duplicate code in the DataModules and thus makes maintaining them easier.

Fixes #395

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

Is this pull request ready for review? (if not, please submit in draft mode)

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 🙃

codecov · 2020-11-24T13:42:08Z

Codecov Report

Merging #400 (4cf20b6) into master (347a63d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #400   +/-   ##
=======================================
  Coverage   80.40%   80.40%           
=======================================
  Files         101      101           
  Lines        5685     5685           
=======================================
  Hits         4571     4571           
  Misses       1114     1114

Flag	Coverage Δ
cpu	`24.98% <ø> (ø)`
pytest	`24.98% <ø> (ø)`
unittests	`79.96% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 347a63d...4cf20b6. Read the comment docs.

nateraw

nice work 😄 . couple notes.

nateraw · 2020-11-24T22:01:59Z

pl_bolts/datamodules/base_datamodule.py

+        num_workers: int = 16,
+        normalize: bool = False,
+        seed: int = 42,
+        batch_size: int = 32,


I like the idea of eval_batch_size and train_batch_size being separate

This would break a lot of tests. Should I still do it?

Let's move this change to separate PR?
in theory, how much the train and valid batch size can have an effect?

pl_bolts/datamodules/base_datamodule.py

Borda · 2020-11-26T18:26:40Z

@ananyahjha93 @nateraw is it ready to go?

chris-clem · 2020-12-03T19:22:31Z

Any updates?

akihironitta

@chris-clem It looks good to me.
@nateraw @ananyahjha93 Can you also have a look?

Borda · 2020-12-08T08:26:24Z

@chris-clem can you pls resolve conflits?

chris-clem · 2020-12-09T16:39:24Z

@chris-clem can you pls resolve conflits?

I just did it

Borda · 2020-12-14T21:12:58Z

@chris-clem we have merged better importing with if/else instead of try/except mind pls update the PR..
sorry for the inconvenience, but I think it shall land soon :]

Borda

could we pls also add typing?

pl_bolts/datamodules/base_datamodule.py

chris-clem · 2020-12-15T16:07:20Z

@chris-clem we have merged better importing with if/else instead of try/except mind pls update the PR..
sorry for the inconvenience, but I think it shall land soon :]

No problem, I updated the PR

Borda

I will implement some other comments as direct commits here

pl_bolts/datamodules/base_datamodule.py

pl_bolts/datamodules/binary_mnist_datamodule.py

…hvision is not installed there)

Borda · 2020-12-16T16:08:31Z

@chris-clem @akihironitta I have made some more suggestions and rebased on master, mind check it...

akihironitta

@chris-clem @Borda The changes basically look great to me! I left some comments, so could you have a look?

pl_bolts/datamodules/binary_mnist_datamodule.py

pl_bolts/datamodules/vision_datamodule.py

pl_bolts/datamodules/cifar10_datamodule.py

pl_bolts/datamodules/vision_datamodule.py

Co-authored-by: Akihiro Nitta <[email protected]>

Borda · 2020-12-17T14:48:20Z

@chris-clem mind resolve conflicts and #400 (comment)

CHANGELOG.md

pl_bolts/datamodules/binary_mnist_datamodule.py

# Conflicts: # pl_bolts/datamodules/binary_mnist_datamodule.py # pl_bolts/datamodules/cifar10_datamodule.py # pl_bolts/datamodules/fashion_mnist_datamodule.py # pl_bolts/datamodules/mnist_datamodule.py

… into feature/395_refactor-vision-dms # Conflicts: # pl_bolts/datamodules/cifar10_datamodule.py # pl_bolts/datamodules/fashion_mnist_datamodule.py # pl_bolts/datamodules/mnist_datamodule.py

pl_bolts/datamodules/cifar10_datamodule.py

… into feature/395_refactor-vision-dms

pl_bolts/datamodules/cifar10_datamodule.py

pl_bolts/datamodules/vision_datamodule.py

akihironitta

@chris-clem It looks valid to me :]

Borda requested a review from nateraw November 24, 2020 13:46

ananyahjha93 self-requested a review November 24, 2020 16:40

nateraw reviewed Nov 24, 2020

View reviewed changes

chris-clem requested a review from nateraw November 25, 2020 15:38

akihironitta added datamodule Anything related to datamodules refactoring labels Nov 25, 2020

Borda requested a review from akihironitta December 5, 2020 00:59

akihironitta approved these changes Dec 5, 2020

View reviewed changes

chris-clem force-pushed the feature/395_refactor-vision-dms branch from eb01b40 to b17a5d8 Compare December 9, 2020 15:31

chris-clem requested a review from Borda December 10, 2020 08:25

Borda reviewed Dec 14, 2020

View reviewed changes

pl_bolts/datamodules/base_datamodule.py Outdated Show resolved Hide resolved

Borda self-assigned this Dec 14, 2020

chris-clem requested a review from Borda December 15, 2020 16:07

Borda reviewed Dec 16, 2020

View reviewed changes

pl_bolts/datamodules/base_datamodule.py Outdated Show resolved Hide resolved

pl_bolts/datamodules/base_datamodule.py Outdated Show resolved Hide resolved

pl_bolts/datamodules/binary_mnist_datamodule.py Outdated Show resolved Hide resolved

chris-clem added 8 commits December 16, 2020 16:59

Add BaseDataModule

d5d4dda

Add pre-commit hooks

4876be0

Refactor cifar10_datamodule

9476033

Move torchvision warning

0651131

Refactor binary_mnist_datamodule

e1f6238

Refactor fashion_mnist_datamodule

5b9e2fd

Fix errors

9ab0640

Remove VisionDataset type hint so CI base testing does not fail (torc…

b7840bf

…hvision is not installed there)

akihironitta suggested changes Dec 17, 2020

View reviewed changes

Apply suggestions from code review

716dedf

Co-authored-by: Akihiro Nitta <[email protected]>

chris-clem added 2 commits December 17, 2020 18:18

Add flags from Lightning-Universe#388 to API

54114c5

Make tests work

be6fb25

chris-clem commented Dec 17, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

chris-clem commented Dec 17, 2020

View reviewed changes

pl_bolts/datamodules/binary_mnist_datamodule.py Show resolved Hide resolved

chris-clem and others added 5 commits December 17, 2020 18:56

Merge branch 'master' into feature/395_refactor-vision-dms

a55ad63

Move _TORCHVISION_AVAILABLE check

25382a2

Update changelog

4902c1c

Merge branch 'master' into feature/395_refactor-vision-dms

2d29a1f

# Conflicts: # pl_bolts/datamodules/binary_mnist_datamodule.py # pl_bolts/datamodules/cifar10_datamodule.py # pl_bolts/datamodules/fashion_mnist_datamodule.py # pl_bolts/datamodules/mnist_datamodule.py

Merge remote-tracking branch 'origin/feature/395_refactor-vision-dms'…

26eafdf

… into feature/395_refactor-vision-dms # Conflicts: # pl_bolts/datamodules/cifar10_datamodule.py # pl_bolts/datamodules/fashion_mnist_datamodule.py # pl_bolts/datamodules/mnist_datamodule.py

chris-clem commented Dec 17, 2020

View reviewed changes

pl_bolts/datamodules/cifar10_datamodule.py Outdated Show resolved Hide resolved

chris-clem requested review from Borda and akihironitta December 17, 2020 18:26

chris-clem added 3 commits December 17, 2020 19:35

Fix CI base testing

875f13a

Fix CI base testing

aac84ab

Merge remote-tracking branch 'origin/feature/395_refactor-vision-dms'…

1d5f0b5

… into feature/395_refactor-vision-dms

akihironitta reviewed Dec 17, 2020

View reviewed changes

pl_bolts/datamodules/cifar10_datamodule.py Outdated Show resolved Hide resolved

pl_bolts/datamodules/vision_datamodule.py Outdated Show resolved Hide resolved

pl_bolts/datamodules/vision_datamodule.py Outdated Show resolved Hide resolved

Apply suggestions from code review

4cf20b6

akihironitta approved these changes Dec 17, 2020

View reviewed changes

Borda approved these changes Dec 17, 2020

View reviewed changes

Borda merged commit 02684ef into Lightning-Universe:master Dec 17, 2020

chris-clem deleted the feature/395_refactor-vision-dms branch December 18, 2020 07:31

Borda added this to the v0.3 milestone Jan 18, 2021

This was referenced Mar 13, 2021

[tune](deps): Bump pytorch-lightning-bolts from 0.2.5 to 0.3.0 in /python/requirements suquark/ray#8

Closed

[tune](deps): Bump pytorch-lightning-bolts from 0.2.5 to 0.3.0 in /python/requirements sven1977/ray#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Vision DataModules #400

Refactor Vision DataModules #400

chris-clem commented Nov 24, 2020 •

edited

Loading

codecov bot commented Nov 24, 2020 •

edited

Loading

nateraw left a comment

nateraw Nov 24, 2020

chris-clem Nov 25, 2020

Borda Dec 5, 2020

Borda commented Nov 26, 2020

chris-clem commented Dec 3, 2020

akihironitta left a comment

Borda commented Dec 8, 2020

chris-clem commented Dec 9, 2020

Borda commented Dec 14, 2020

Borda left a comment

chris-clem commented Dec 15, 2020

Borda left a comment

Borda commented Dec 16, 2020

akihironitta left a comment

Borda commented Dec 17, 2020

akihironitta left a comment

Refactor Vision DataModules #400

Refactor Vision DataModules #400

Conversation

chris-clem commented Nov 24, 2020 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Nov 24, 2020 • edited Loading

Codecov Report

nateraw left a comment

Choose a reason for hiding this comment

nateraw Nov 24, 2020

Choose a reason for hiding this comment

chris-clem Nov 25, 2020

Choose a reason for hiding this comment

Borda Dec 5, 2020

Choose a reason for hiding this comment

Borda commented Nov 26, 2020

chris-clem commented Dec 3, 2020

akihironitta left a comment

Choose a reason for hiding this comment

Borda commented Dec 8, 2020

chris-clem commented Dec 9, 2020

Borda commented Dec 14, 2020

Borda left a comment

Choose a reason for hiding this comment

chris-clem commented Dec 15, 2020

Borda left a comment

Choose a reason for hiding this comment

Borda commented Dec 16, 2020

akihironitta left a comment

Choose a reason for hiding this comment

Borda commented Dec 17, 2020

akihironitta left a comment

Choose a reason for hiding this comment

chris-clem commented Nov 24, 2020 •

edited

Loading

codecov bot commented Nov 24, 2020 •

edited

Loading