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

DCGAN model for Flux v0.10 #207

Merged
merged 10 commits into from
Mar 2, 2020
Merged

Conversation

matsueushi
Copy link
Contributor

This is a DCGAN implementation for Flux v0.10. I know there are already pending pull requests for DCGAN (#47, #111), but they are incompatible with Zygote.

A linear layer is used as the last layer of the discriminator and losses are calculated using logitbinarycrossentropy. This is because the combination of sigmoid and binarycrossentropy may cause numerical issues (FluxML/Flux.jl#914).

It ouputs generated digits for a fixed noise every 1000 iterations. I believe it is helpful to trace its training process :)

0 steps
dcgan_steps_000000

3000 steps
dcgan_steps_003000

6000 steps
dcgan_steps_006000

final result (=9380 steps)
dcgan_steps_009380

end

cd(@__DIR__)
train()
Copy link
Member

Choose a reason for hiding this comment

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

newline here

Copy link
Member

Choose a reason for hiding this comment

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

sorry, I meant a newline at the very end of the file, so that we don't have github complaining with a red arrow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry. I'll fix that

@CarloLucibello
Copy link
Member

very nice. Is this the architecture from the original paper (or any other)?. If so, can you add a reference in a comment?

@CarloLucibello
Copy link
Member

you should add the model to the README

@CarloLucibello
Copy link
Member

CarloLucibello commented Mar 1, 2020

Maybe this is the right moment to start reorganizing this repo. We could have a folder structure like this domain/model_dataset/, and each example should come with a Project.toml and a Manifest.toml.
So, this should go under vision/dcgan_mnist/.
Is this too cumbersome? @matsueushi what do you think?

@info("Train step $(train_steps), Discriminator loss = $(loss_dscr), Generator loss = $(loss_gen)")
# Save generated fake image
output_image = create_output_image(gen, fixed_noise)
save(@sprintf("dcgan_steps_%06d.png", train_steps), output_image)
Copy link
Member

Choose a reason for hiding this comment

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

you could also add to this PR those figures you produced, it's nice to have a reference

@CarloLucibello
Copy link
Member

Could you also use https://github.com/JuliaML/MLDatasets.jl instead of Flux.Data.MNIST? It's more future proof since we are going to excise Julia.Data soon

@matsueushi
Copy link
Contributor Author

@CarloLucibello , thank you for reviewing my PR.

very nice. Is this the architecture from the original paper (or any other)?. If so, can you add a reference in a comment?

Basically the architecture follows the DCGAN tutorial for tensorflow (https://www.tensorflow.org/tutorials/generative/dcgan). I would change the hyperparams if necessary.

Maybe this is the right moment to start reorganizing this repo. We could have a folder structure like this domain/model_dataset/, and each example should come with a Project.toml and a Manifest.toml.
So, this should go under vision/dcgan_mnist/.
Is this too cumbersome? @matsueushi what do you think?

I'm on board. It will be easier to maintain models and update deps.

@matsueushi
Copy link
Contributor Author

I will replace Flux.Data.MNIST by MLDatasets

using Printf

const BATCH_SIZE = 128
const NOISE_DIM = 100
Copy link
Member

Choose a reason for hiding this comment

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

maybe LATENT_DIM sounds better here

@CarloLucibello
Copy link
Member

I very much like the overall style, this would be a nice template for the other models.
My only perplexity is on the use of global variables: I have a REPL based workflow and typically have all parameters as keyword arguments to the main function, then pass them around to the various methods, usually grouping them together in a Params struct I create with https://github.com/mauro3/Parameters.jl. Of course, this is just my personal taste, what you did
here is perfectly fine, just wondering what would be the "julian" way to write deep learning scripts that most people would consider as convenient templates for their code

@matsueushi
Copy link
Contributor Author

matsueushi commented Mar 1, 2020

I used global variables just because they are used in other vision models. So if it is time to change, I would switch them to Params.

@matsueushi
Copy link
Contributor Author

Params is already used by Zygote (https://github.com/FluxML/Zygote.jl/blob/6f17f8b5c40e48e6d7732afb91dba9a1ddac145b/src/compiler/interface.jl#L53-L57) so I used HyperParams for struct


function train()
# Model Parameters
hparams = HyperParams()
Copy link
Member

Choose a reason for hiding this comment

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

here we can do:

function train(; kws...)
    hparams = HyperParams(; kws...)
    ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

# Load MNIST dataset
images, _ = MLDatasets.MNIST.traindata(Float32)
# Normalize to [-1, 1] and convert it to WHCN
image_tensor = permutedims(reshape(@.(2f0 * images - 1f0), 28, 28, 1, :), (2, 1, 3, 4))
Copy link
Member

Choose a reason for hiding this comment

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

ouch, the permutedims is a bit annoying but I guess we cannot do much about it without breaking changes in MLDatasets

Copy link
Member

Choose a reason for hiding this comment

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

good catch anyways

@CarloLucibello
Copy link
Member

could you commit the pngs as well? then I think we are good to go

@matsueushi
Copy link
Contributor Author

could you commit the pngs as well? then I think we are good to go

Sure, I will rerun a whole training process again to make sure everything works fine and add ouput images

@matsueushi
Copy link
Contributor Author

0 steps
dcgan_steps_000000

3000 steps
dcgan_steps_003000

6000 steps
dcgan_steps_006000

9380 steps
dcgan_steps_009380

generator_loss(fake_output) = mean(logitbinarycrossentropy.(fake_output, 1f0))

function train_discriminator!(gen, dscr, batch, opt_dscr, hparams)
noise = randn(Float32, hparams.latent_dim, hparams.batch_size) |> gpu
Copy link
Member

Choose a reason for hiding this comment

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

this is not ideal, we should avoid data transfer and use CuArrays.randn if training on gpu. I don't know what would be an elegant way to do that, maybe we can revisit in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

nice!

for batch in data
# Update discriminator and generator
loss_dscr = train_discriminator!(gen, dscr, batch, opt_dscr, hparams)
loss_gen = train_generator!(gen, dscr, batch, opt_gen, hparams)
Copy link
Member

Choose a reason for hiding this comment

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

here part of the computation done in train_discriminator! could be reused in train_generator!, i.e. in principle one need only a single forward of the generator.

This is also done here https://pytorch.org/tutorials/beginner/dcgan_faces_tutorial.html

I don't know how to do it properly with zygote, needs some thinking. If you have a simple solution in mind you could add this optimization, otherwise we can revisit in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately I don't know how to do it, too.

@CarloLucibello
Copy link
Member

better move the images to a subfolder

@CarloLucibello
Copy link
Member

last tiny detail, consider renaming batch to x

@matsueushi
Copy link
Contributor Author

Thanks, I applied the changes

@CarloLucibello
Copy link
Member

terrific works, thanks!

@CarloLucibello CarloLucibello merged commit 5c289c9 into FluxML:master Mar 2, 2020
@matsueushi
Copy link
Contributor Author

@CarloLucibello, thank you for your awesome detailed review!

@matsueushi matsueushi deleted the mnist-dcgan branch March 2, 2020 12:01
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.

None yet

2 participants