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

🧹 Cleanup of DCGAN & CVAE #1387

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lindapaiste
Copy link
Contributor

Extends PR #1376 -- merge that first.
Fixes #949
Possible resolves #1386, subject to discussion.

CVAE

  • generate() accepts an optional latentVector (inspired by DCGAN).
  • Throw an error if the label cannot be found instead of returning nonsensical garbage [undefined, undefined].
  • Remove hard-coded input size of [1, 16] as this will vary based on the model. Accesses the input shape by looking at the model instead.
  • Remove unnecessary instance properties this.labelVector and this.latentDim. These are now local variables in generate().
  • Make use of built tensorflow functions:
    • tf.oneHot() to encode the label input
    • .squeeze() instead of verbose .reshape([temp.shape[1], temp.shape[2], temp.shape[3]])
  • Should use property raw instead of raws to match other models. Now returns both to be non-breaking.
  • Better JSDoc -- I need to find a system that can sync these JSDoc types and description into our documentation website.
  • Inline generateInternal() as an IIFE inside of generate() so that I didn't need to duplicate the JSDoc.
  • Exhaustive unit tests.

DCGAN

  • Can accept either a model.json or a manifest.json, since the manifest does not provide any necessary info. (see issue Discussion: does DCGAN really need a manifest.json? #1386)
  • generate() can accept both a callback and a latentVector with both optional and in any order. Callbacks should always be last so the way that we had it did not make sense.
  • Combine generate(), generateInternal() and compute() into one method.
  • Offload the first 10 lines of compute() into a utility.
  • Exhaustive unit tests.

tensorInput

  • New utility file handles common logic in both CVAE and DCGAN (and potentially others in the future).
  • modelInputShape figures out the correct tensor shape based on the model. This is really important and should be used in a few other models as well. Having hard-coded variables like IMAGE_SIZE will not work if the user provides a URL to a custom model which uses a different input size than what is expected by the code.
    • Argument layerIndex allows this to work for models like CVAE which take an array of tensors instead of a single tensor input.
  • validateLatentInput includes the existing logic which I removed from DCGAN.compute() for creating either a random input tensor or a tensor from a provided array.
    • Adds an error message if the array size is wrong. This should be slightly more helpful than the TF error that would get thrown on the next line ("Uncaught Error: Based on the provided shape, [2,2], the tensor should have 4 values but has 6").
    • The user can now provide a Tensor in addition to an array.

Failed Tests

There are 2 failures in the new tests that I've added:

  1. Loading a DCGAN from the current geo manifest.json does not work due to the relative URL issue discussed in Discussion: does DCGAN really need a manifest.json? #1386
  2. Generating a P5 image fails. I added P5 as a devDependency so that we can do tests like this. However, calling p5Utils.setP5Instance() does not work as intended. I believe that there is an issue in the P5Util class.

# Conflicts:
#	src/CVAE/index.js
#	src/DCGAN/index.js
#	src/FaceApi/index.js
#	src/SketchRNN/index.js
Copy link
Contributor

@joeyklee joeyklee left a comment

Choose a reason for hiding this comment

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

✅ LGTM -- there's a lot in here!

Loading a DCGAN from the current geo manifest.json does not work due to the relative URL issue discussed in #1386

  • It seems like you figured out a way to get back the info that we need from the manifest.json -- tbh, I'm not sure what other info we expect to be in there. I wonder if those were old files that were part of models exported from models trained in tensorflow or keras or something 😬
  • on the point of relative imports -- the original intention was just to try to make things as easy as possible for people to load models in. I don't think we had a plan other than it seemed ok at the time. I think I'm fine to defer to you to roll out your plan!

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Discussion: does DCGAN really need a manifest.json? DCGAN example error
2 participants