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

Add DCGAN module #403

Merged
merged 62 commits into from
Jan 18, 2021
Merged

Conversation

chris-clem
Copy link
Contributor

@chris-clem chris-clem commented Nov 24, 2020

What does this PR do?

As proposed in #401, this PR adds a DCGAN implementation closely following the one in PyTorch's examples (https://github.com/pytorch/examples/blob/master/dcgan/main.py).

Fixes #401

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
Copy link

codecov bot commented Nov 24, 2020

Codecov Report

Merging #403 (01fea8a) into master (413b9df) will decrease coverage by 0.03%.
The diff coverage is 97.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #403      +/-   ##
==========================================
- Coverage   79.49%   79.46%   -0.04%     
==========================================
  Files         102      104       +2     
  Lines        5912     6047     +135     
==========================================
+ Hits         4700     4805     +105     
- Misses       1212     1242      +30     
Flag Coverage Δ
cpu 25.86% <29.85%> (+0.09%) ⬆️
pytest 25.86% <29.85%> (+0.09%) ⬆️
unittests 78.98% <97.77%> (-0.03%) ⬇️

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

Impacted Files Coverage Δ
pl_bolts/callbacks/vision/image_generation.py 91.30% <ø> (ø)
pl_bolts/models/gans/dcgan/dcgan_module.py 97.22% <97.22%> (ø)
pl_bolts/models/gans/__init__.py 100.00% <100.00%> (ø)
pl_bolts/models/gans/dcgan/components.py 100.00% <100.00%> (ø)
pl_bolts/datasets/cifar10_dataset.py 71.73% <0.00%> (-26.09%) ⬇️
pl_bolts/datasets/base_dataset.py 81.81% <0.00%> (-13.64%) ⬇️
...l_bolts/models/rl/vanilla_policy_gradient_model.py 96.36% <0.00%> (+2.72%) ⬆️

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 413b9df...e2f6f46. Read the comment docs.

@chris-clem chris-clem changed the title WIP: Add DCGAN module Add DCGAN module Nov 25, 2020
@chris-clem chris-clem marked this pull request as ready for review November 25, 2020 16:36
@akihironitta akihironitta added this to In progress in AE & GANs via automation Nov 25, 2020
@Borda Borda added the enhancement New feature or request label Nov 26, 2020
pl_bolts/models/gans/dcgan/components.py Outdated Show resolved Hide resolved
@chris-clem
Copy link
Contributor Author

The tests fail because CIFAR10 fails to download. Is it possible to cache it? Also: is it possible to deactivate some of the tests in the beginning? It feels quite resource-draining.

And why does codefactor check a file that I haven't changed (docs\source\conf.py)?

@akihironitta
Copy link
Contributor

@chris-clem Thank you for your contribution!

The tests fail because CIFAR10 fails to download. Is it possible to cache it?

Actually, we've recently enabled dataset caching in #315, but it seems it is not working properly as you point out...
Just re-ran the ci, let's see how it goes... https://github.com/PyTorchLightning/pytorch-lightning-bolts/runs/1462875014

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

Also please make sure to add the summary of your change to CHANGELOG.md :]

pl_bolts/models/gans/dcgan/dcgan_module.py Show resolved Hide resolved
tests/models/test_gans.py Outdated Show resolved Hide resolved
AE & GANs automation moved this from In progress to in Review Nov 27, 2020
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for your swift action!

@chris-clem
Copy link
Contributor Author

Was it wrong to rebase?

LGTM. Thank you for your swift action!

Thanks, happy to contribute!

@akihironitta
Copy link
Contributor

akihironitta commented Nov 27, 2020

@chris-clem

Was it wrong to rebase?

I believe it's fine. Let me know if there is something that concerns you :]

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@chris-clem
Copy link
Contributor Author

@chris-clem Could you also add DCGAN to the docs like GAN? https://github.com/PyTorchLightning/pytorch-lightning-bolts/blob/master/docs/source/gans.rst

Sorry for the delay. I get access to a GPU server in a couple of days and will train a model there.

chris-clem and others added 3 commits December 15, 2020 17:10
…verse#344)

* bugfix: batch_size for DataModules remaining

* Update sklearn datamodule tests

* Fix default_transforms. Keep internal for every data module

* fix typo on binary_mnist_datamodule

thanks @akihironitta

Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>

Co-authored-by: Akihiro Nitta <nitta@akihironitta.com>
@chris-clem
Copy link
Contributor Author

@akihironitta Here also the ubuntu and mac python 3.6 tests fail.

@ananyahjha93
Copy link
Contributor

@chris-clem same cause #471. Taking a look.

@akihironitta
Copy link
Contributor

@akihironitta Here also the ubuntu and mac python 3.6 tests fail.

@chris-clem Thank you for quickly applying the suggestion. As @ananyahjha93 mentioned, the other failures in the tests have been reported in #471.

@ananyahjha93
Copy link
Contributor

@chris-clem rebase and resolve conflicts, good to go!

@akihironitta akihironitta self-requested a review January 7, 2021 10:44
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

lgtm

@akihironitta
Copy link
Contributor

@chris-clem Would you mind resolving the conflict and applying yapf to pass the tests in CI? The other failing tests seem irrelevant to the changes in this PR, so I think we can land this once the formatting is resolved.

@akihironitta akihironitta self-assigned this Jan 17, 2021
@chris-clem
Copy link
Contributor Author

@chris-clem Would you mind resolving the conflict and applying yapf to pass the tests in CI? The other failing tests seem irrelevant to the changes in this PR, so I think we can land this once the formatting is resolved.

@akihironitta Done :)!

Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

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

@chris-clem Thank you for your swift action!

@Borda
Copy link
Member

Borda commented Jan 18, 2021

@chris-clem @akihironitta can you pls resolve conflicts (rebase) so we can get it land...
seems that yapf is missing in this PR

@chris-clem
Copy link
Contributor Author

@chris-clem @akihironitta can you pls resolve conflicts (rebase) so we can get it land...
seems that yapf is missing in this PR

@Borda I don't see any conflicts at the moment and I added yapf yesterday. The yapf tests passes. Am I missing something else?

@Borda
Copy link
Member

Borda commented Jan 18, 2021

@chris-clem the conflicts appeared when I tried to rebase it so we test it out with newly added py 3.8
But let's see, it shall be fine =)

CHANGELOG.md Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request model ready
Projects
Development

Successfully merging this pull request may close these issues.

Add DCGAN