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

GAN example: Only one backward() call? #594

Closed
alainjungo opened this issue Dec 5, 2019 · 7 comments · Fixed by #1269
Closed

GAN example: Only one backward() call? #594

alainjungo opened this issue Dec 5, 2019 · 7 comments · Fixed by #1269
Assignees
Labels
feature Is an improvement or enhancement let's do it! approved to implement question Further information is requested

Comments

@alainjungo
Copy link

In the PyTorch GAN tutorial https://pytorch.org/tutorials/beginner/dcgan_faces_tutorial.html there are two backward() calls for the discriminator. How do you ensure this with your structure, where backward() gets called after the training step?

Best,
Alain

@alainjungo alainjungo added the question Further information is requested label Dec 5, 2019
@williamFalcon
Copy link
Contributor

williamFalcon commented Dec 7, 2019

good catch. we don't actually support this atm.

A solution here is to allow a dictionary of options for each optimizer which allows arbitrary number of calls.
Something like:

def configure_optimizers(self,...):
    opt_G = {'optimizer': Adam(...), 'frequency': 2, 'lr_scheduler': LRScheduler(...)}
    opt_D = {'optimizer': Adam(...), 'frequency': 1, 'lr_scheduler': LRScheduler(...)}

    return opt_G, opt_D

Here G would be called twice back to back, and G once after

@jeffling @neggert

But not sure if this is a clean user experience

@jeffling
Copy link
Contributor

jeffling commented Dec 9, 2019

@williamFalcon that API would work for us.

@alainjungo: Some workarounds, none of them ideal:

  1. Skip every other generator step. You'll have to double your iterations
  2. Double learning rate on D (not algorithmically the same but can have similar effect)

@kwanUm
Copy link

kwanUm commented Jan 2, 2020

You can always return one loss at the training step that captures both losses.
In the dcgan example, this would look like errD = errD_real + errD_fake; errD.backword();.

Not sure if I'm correct here, but this seems equivalent and matches PTL paradigms.

@jeffling jeffling added the feature Is an improvement or enhancement label Jan 14, 2020
@asafmanor
Copy link
Contributor

I have implemented an API that allows returning optimizer, lr_schedulers, optimizer_frequencies,
and then based on the batch_idx will determine the current optimizer to use.

If there is an agreement on this API, I'll proceed to testing, documenting and submitting a PR.

Another option would be to allow returning a tuple of dictionaries as @williamFalcon suggested. that would be a minor change for me and I am willing to that if it is agreed upon.

@asafmanor
Copy link
Contributor

asafmanor commented Mar 15, 2020

I am adding here that the official implementation of WGAN-GP (for example) needs this feature in order to converge.
This is a very fundamental feature in GAN training.
image

@Borda Borda added this to the 0.7.3 milestone Mar 26, 2020
@Borda
Copy link
Member

Borda commented Mar 26, 2020

I like the @williamFalcon which seems clear to me...
@asafmanor mind sens a PR or describe what API you have in mind?
cc: @PyTorchLightning/core-contributors any comments on this?

@Borda Borda added the let's do it! approved to implement label Mar 26, 2020
@asafmanor
Copy link
Contributor

I'll implement the @williamFalcon API and send a detailed PR over the weekend 👍

@Borda Borda removed this from the 0.7.3 milestone Mar 26, 2020
@mergify mergify bot closed this as completed in #1269 Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement let's do it! approved to implement question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants