Skip to content

Add variable number of discriminator updates for each generator update#103

Merged
fealho merged 6 commits intosdv-dev:masterfrom
Baukebrenninkmeijer:wgan-critic-implementation
Dec 3, 2020
Merged

Add variable number of discriminator updates for each generator update#103
fealho merged 6 commits intosdv-dev:masterfrom
Baukebrenninkmeijer:wgan-critic-implementation

Conversation

@Baukebrenninkmeijer
Copy link
Copy Markdown
Contributor

@Baukebrenninkmeijer Baukebrenninkmeijer commented Dec 2, 2020

As per WGAN, the discriminator should be updated 5 times more often than the generator. This is added as a parameter in the __init__ function.

Fixes #101

@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Dec 2, 2020

CLA assistant check
All committers have signed the CLA.

Comment thread ctgan/synthesizer.py Outdated
Copy link
Copy Markdown
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

Thanks @Baukebrenninkmeijer It looks good!

I would make a couple of minor changes that I think will improve the overall result:

  1. I would rename the parameter to something like discriminator_steps. Otherwise, at first sight someone may thing that n_discriminators=5 means "Use 5 different discriminators".

  2. Providing some more context in the docstring would be helpful. I would add a sentence to explain what is the effect of increasing or decreasing the value, a link to the WGAN paper, and then mention that the default is 1 to match the original CTGAN implementation.

Would you mind adding these? Once it is done I think we are all set!

@Baukebrenninkmeijer
Copy link
Copy Markdown
Contributor Author

@csala Done and done

Copy link
Copy Markdown
Contributor

@csala csala left a comment

Choose a reason for hiding this comment

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

Looks good now! Thanks @Baukebrenninkmeijer !

@Baukebrenninkmeijer
Copy link
Copy Markdown
Contributor Author

Some checks are not clearing due to unrelated requirements issues it seems.

@csala @fealho

@csala
Copy link
Copy Markdown
Contributor

csala commented Dec 3, 2020

Some checks are not clearing due to unrelated requirements issues it seems.

Yes, sometimes there are random errors related to the Github Actions infrastructure. I'm re-running the tests.

@Baukebrenninkmeijer
Copy link
Copy Markdown
Contributor Author

Hmmm a value count in the samples was too high. But that seems also unrelated with my changes. Is that bound maybe too tight? Its 6550isch when it should be below 6500.

@fealho fealho merged commit 0c078af into sdv-dev:master Dec 3, 2020
@Baukebrenninkmeijer Baukebrenninkmeijer deleted the wgan-critic-implementation branch December 4, 2020 09:23
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.

Discriminator should train more than generator

4 participants