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

Refactor basic GAN tutorial #357

Merged
merged 4 commits into from
Aug 1, 2024
Merged

Refactor basic GAN tutorial #357

merged 4 commits into from
Aug 1, 2024

Conversation

jhauret
Copy link
Contributor

@jhauret jhauret commented Aug 1, 2024

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?

What does this PR do?

Fixes #306

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?

I had ! Glad to contribute to the best Pytorch wrapper 🙃

Notes

  • The quality of the generated samples remains low, but I don't have the time to tune the architecture more. Also, applying tricks to reach Nash equilibrium like label smoothing could make the example less understandable, so I left it as it is.

  • This PR was motivated in the case of some heavy generator and discriminator that we don't want to call multiple times. I removed the repeated calls to the generator (which changes the optimization problem a bit, because the discriminator is trained to detect the same sample that the generator just generated) BUT I did not remove the repeated calls to the discriminator. In fact, trying self.manual_backward(g_loss, retain_graph=True) caused an error when backpropagating on the discriminator loss because the generator weights changed (RuntimeError: one of the variables needed for gradient computation has been modified by an inplace operation). If anyone knows how to remove the two calls to the discriminator, I'm curious!

  • Logging is fixed and on_validation_epoch_end is now called

@Borda Borda requested a review from awaelchli August 1, 2024 14:56
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70%. Comparing base (d85a312) to head (c39a740).

Additional details and impacted files
@@         Coverage Diff         @@
##           main   #357   +/-   ##
===================================
  Coverage    70%    70%           
===================================
  Files         2      2           
  Lines       441    441           
===================================
  Hits        307    307           
  Misses      134    134           

@Borda
Copy link
Member

Borda commented Aug 1, 2024

Thank you and @jhauret if you would like to continue with the refactoring any time in the future, all help is welcome! 🐰

@Borda Borda merged commit b09694f into Lightning-AI:main Aug 1, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repeated calls to generator and discriminator's forward in GAN tutorial
2 participants