gan icon indicating copy to clipboard operation
gan copied to clipboard

Update losses module

Open SauravMaheshkar opened this issue 4 years ago • 6 comments

This is in reference to issue #24. I was assigned to make the Tutorial Notebook compatible with Keras instead of the Estimator API.

I've already made a new input batch generator and Model Architectures using tfv2 functionalities. But I noticed that the losses module also uses certain tfv1 functionalities. For example, the losses_impl.py file uses tf.compat.v1.name_scope instead of tf.name_scope. I'd like to update the losses module to be tfv2 compatible.

I'm willing to contribute. Please assign this to me.

SauravMaheshkar avatar Oct 19 '20 04:10 SauravMaheshkar

Hi Saurav,

Thanks for contributing!

Pretty much this whole library is designed around TF1 constructs and in order to get it working with Keras and proper TF2 it probably needs a well thought out wholistic approach. We're also not ready to drop TF1 support, so we either need to have 2 versions of the code or else a way to keep things backwards compatible. The GANModel data structure, which is sort of the center of the whole library relies heavily on variable_scopes, which don't exist in TF2, so that might be the first thing to figure out, and then figure out how to adapt everything else from there.

Would you be interested in coming up with a plan of that sort, and then maybe a proof-of-concept?

The particular issue you point out here could be just one step of the larger plan.

aaronsarna avatar Oct 19 '20 16:10 aaronsarna

Hey @aaronsarna, that's a great suggestion. I was looking into the variable_scopes as well while attempting to redefine the wasserstein_loss function. I'll get started on building up a plan for converting v1.get_variable to tf.Variable and similarly to convert v1.variable_scope to other keras objects like layers/models/module

SauravMaheshkar avatar Oct 19 '20 17:10 SauravMaheshkar

@aaronsarna do you want me to use a template for the Proof-of-concept?

SauravMaheshkar avatar Oct 20 '20 14:10 SauravMaheshkar

I think just showing a basic mnist or cifar10 GAN trained with TF2 and Keras is sufficient.

aaronsarna avatar Oct 20 '20 14:10 aaronsarna

The GANModel class uses the collections module for creating tuple subclasses with named fields. But as per the docs page, this module has been "Deprecated since version 3.3, will be removed in version 3.10". Would you advise me to use the collections.abc module instead or did you have another module in mind?

SauravMaheshkar avatar Oct 21 '20 11:10 SauravMaheshkar

I don't think there's any need to stick with collections.namedtuple. To some extent the right answer will depend on the overall design. It may just be a class, or you could look at using attr or dataclasses. Whatever makes the most sense given the overall design.

aaronsarna avatar Oct 21 '20 13:10 aaronsarna