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

Update for Pytorch 1.7 #126

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Update for Pytorch 1.7 #126

wants to merge 6 commits into from

Conversation

LeviDPC
Copy link

@LeviDPC LeviDPC commented Nov 6, 2020

This project has a bug that has kept it from running on versions of Pytorch 1.5 or after. I'm pretty sure this is because in Pytorch 1.5 one of the bug fixes was: torch.optim optimizers changed to fix in-place checks for the changes made by the optimizer. If you want to know more find the patch notes and do a search for those key words.

A different pull request had a fix that ran but ended up with bad picture output.

The supplementary materials for SinGAN list one of the optimizations as alternating between 3 gradient steps for the generator and 3 gradient steps for the discriminator. This is different from the basic Pytorch DCGAN Tutorial (which SinGANs code seems to be partially based on) where this isn't the case and it simply alternates between generator and discriminator steps. It appears that the bug is coming from the OptimizerG step operation performing inplace operations on the Generator parameters. By removing this optimization to make it more like the tutorial I've fixed the bug.

I've done some initial testing and the model seems to work just fine. For performance, my fix seems to go slower initially but then catch up and surpass the current version. If anyone would like to test my changes, they are pushed to my fork of SinGAN: https://github.com/Clefspear99/SinGAN

-Clef

@tamarott
Copy link
Owner

Thank you Clef!
Would be happy to hear if all tests worked fine. Should I push this to the main brunch?

@LeviDPC
Copy link
Author

LeviDPC commented Dec 25, 2020

Hey tamarott,

Yeah, I'm glad to have helped and glad my suggestions didn't break anything further!

If you feel confident then by all means go ahead and push them.

Best,
Clef

@Thierryonre
Copy link

Would push this so that people don't have to go through the issues to find out how to fix the bug - especially new people.

@tamarott
Copy link
Owner

This code seems to do a single D step per G step (in groups of three steps), and is therefore not identical to the original implementation (were 3 D steps are followed by 3 G steps). Identical performance is not granted.

@alicehua11
Copy link

I didn't see this sooner. Spent sometimes trying to fix the problem, would be good to have a fix on this pull request for future people.

@markstrefford
Copy link

Thanks for this @Clefspear99
@tamarott one of the issues with the original code is the version of Torch doesn't work on more recent GPUs, such as the RTX30xx series. I appreciate that changing the approach for steps is different here, but at least this works pull request works with more recent hardware and CUDA/CUDNN versions from NVidia.

@markstrefford
Copy link

For those that want a version that works on more recent Pytorch versions (with the caveats above around not guaranteeing the exact same results), you can find it here https://github.com/Clefspear99/SinGAN (thanks for @Clefspear99 )

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.

5 participants