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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Pix2Pix model #533

Merged
merged 18 commits into from Mar 4, 2021
Merged

Add Pix2Pix model #533

merged 18 commits into from Mar 4, 2021

Conversation

aniketmaurya
Copy link
Contributor

@aniketmaurya aniketmaurya commented Jan 21, 2021

What does this PR do?

Fixes #512

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?
  • Did you make sure to update the documentation with your changes?
  • Did you write any new necessary tests? [not needed for typos/docs]
  • 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 馃檭

@github-actions github-actions bot added the model label Jan 21, 2021
@aniketmaurya aniketmaurya changed the title Draft PR: Add Pix2Pix model: #533 Jan 21, 2021
@aniketmaurya aniketmaurya changed the title Add Pix2Pix model: #533 Add Pix2Pix model Jan 21, 2021
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #533 (1b2b47a) into master (86e3f52) will decrease coverage by 1.07%.
The diff coverage is 21.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #533      +/-   ##
==========================================
- Coverage   77.58%   76.51%   -1.08%     
==========================================
  Files         115      117       +2     
  Lines        6701     6829     +128     
==========================================
+ Hits         5199     5225      +26     
- Misses       1502     1604     +102     
Flag Coverage 螖
cpu 26.12% <21.53%> (-0.12%) 猬囷笍
pytest 26.12% <21.53%> (-0.12%) 猬囷笍
unittests 75.99% <21.53%> (-1.07%) 猬囷笍

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

Impacted Files Coverage 螖
pl_bolts/models/gans/pix2pix/components.py 17.94% <17.94%> (酶)
pl_bolts/models/gans/pix2pix/pix2pix_module.py 22.44% <22.44%> (酶)
pl_bolts/models/gans/__init__.py 100.00% <100.00%> (酶)

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 86e3f52...66317f4. Read the comment docs.

@pep8speaks
Copy link

pep8speaks commented Jan 21, 2021

Hello @aniketmaurya! Thanks for updating this PR.

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

Comment last updated at 2021-02-24 21:04:04 UTC

Copy link
Contributor

@oke-aditya oke-aditya left a comment

Choose a reason for hiding this comment

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

I had glance of it, let me know what you think.
Very nice PR, Pix2Pix is really useful model.


def __init__(self, input_channels, use_dropout=False, use_bn=True):
super(self).__init__()
self.upsample = nn.Upsample(scale_factor=2, mode='bilinear', align_corners=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's just confirm once if Upsample is done using nn.Upsample or nn.ConvTranspose2d both work fine. I haven't read Pix2Pix paper so let me check once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, thank you for the review. In section 6 of the Pix2Pix paper authors have mentioned that they upsampled the tensors by a factor of 2 but they haven't exactly mentioned if Transposed Conv is used or Upsample followed by Conv layer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I quickly checked the paper and found that the PyTorch implementation linked from the author's Lua implementation uses nn.ConvTranspose2d, so shall we follow that architecture unless someone has a strong opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I too confirmed that it is nn.ConvTranspose2d. I have referred TensorFlow docs, which give a really nice implementation.

pl_bolts/models/gans/pix2pix/components.py Outdated Show resolved Hide resolved
Co-authored-by: Aditya Oke <47158509+oke-aditya@users.noreply.github.com>
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.

Thank you for your work! Could you have a look at my nitpicking comments? 馃槄

pl_bolts/models/gans/__init__.py Outdated Show resolved Hide resolved
pl_bolts/models/gans/__init__.py Outdated Show resolved Hide resolved
@akihironitta akihironitta self-assigned this Jan 23, 2021
@aniketmaurya
Copy link
Contributor Author

aniketmaurya commented Jan 30, 2021

Hi, I have added the training step code to the PR. Also I have tried to train the model using Facades dataset.

Please help in review the PR.

Cc: @Borda @akihironitta @oke-aditya

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.

@aniketmaurya Would you mind merging master and applying yapf to pass the failing tests?


def __init__(self, input_channels, use_dropout=False, use_bn=True):
super(self).__init__()
self.upsample = nn.Upsample(scale_factor=2, mode='bilinear', align_corners=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

I quickly checked the paper and found that the PyTorch implementation linked from the author's Lua implementation uses nn.ConvTranspose2d, so shall we follow that architecture unless someone has a strong opinion?

pl_bolts/models/gans/pix2pix/components.py Outdated Show resolved Hide resolved
Copy link
Contributor

@oke-aditya oke-aditya left a comment

Choose a reason for hiding this comment

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

I'm bit unsure about some aspects of implementation.
This colab notebook gives a really simple and effective Pix2Pix.

pl_bolts/models/gans/pix2pix/components.py Outdated Show resolved Hide resolved

def __init__(self, input_channels, use_dropout=False, use_bn=True):
super(self).__init__()
self.upsample = nn.Upsample(scale_factor=2, mode='bilinear', align_corners=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I too confirmed that it is nn.ConvTranspose2d. I have referred TensorFlow docs, which give a really nice implementation.

pl_bolts/models/gans/pix2pix/components.py Outdated Show resolved Hide resolved
@oke-aditya
Copy link
Contributor

GANs are notorius 馃槄 Small differences such as LeakyRelu vs Relu, Dropout probability, ConvTranspose vs Upsampling really make difference. Really tough time in getting a GAN trained, really great job @aniketmaurya

@akihironitta I would suggest to use torchvision, it is well tested and a standard (over 15Million downloads).
Even with lightning flash we use it now 馃槃 It avoids lot of code writing.

@akihironitta
Copy link
Contributor

@oke-aditya I agree, thanks for your comment :)

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.

Hey @aniketmaurya, it'd be great to have this model in Bolts!!! Please mark this PR as ready for review when you're ready :)

requirements.txt Outdated Show resolved Hide resolved
@aniketmaurya aniketmaurya marked this pull request as ready for review February 24, 2021 21:15
@Borda Borda requested a review from akihironitta March 4, 2021 21:46
@Borda Borda merged commit 53f5370 into Lightning-Universe:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Pix2Pix
5 participants