Cleanup 🧹 image argument checking #1322

lindapaiste · 2022-03-08T18:50:42Z

Related issue #1189

handleArguments

Helper function handleArguments constructs and returns an instance of new ArgHelper class.
Reads in an array of unknown arguments and sets each argument to a named property based on its type.
Properties can be destructured as const { video, callback } = args or accessed as args.video.
Returned object has a .require method as a wrapper around error throwing, which is called as args.require('image', 'message when missing').

Utilities

Define boolean type guard functions for various input types (isCanvas, isVideo, etc.).
Define helper function getImageElement to access the underlying DOM element from p5 objects.
Extract common logic from utils flipImage and imgToPixelArray into function drawToCanvas.
Add JSDoc types to some imageUtilities functions.

Models

Use handleArguments to delete large chunks of repeated code from models.
Define JSDoc for option types and result types on BodyPix and CartoonGAN. (the types are still not 100% as VSCode doesn't auto-import @typedef from other files and WebStorm doesn't handle spread types or type guards correctly)

src/utils/handleArguments.js

joeyklee · 2022-03-10T02:10:03Z

src/utils/handleArguments.js

+        }
+        // All other objects are assumed to be options.
+        else {
+          this.options = arg;


Question:

If multiple objects are passed, should we spread all those obj properties into this options object? 😬

That's an interesting idea! I wouldn't want to encourage a situation where the user is providing multiple options objects. But it does open up some interesting possibilities for internal use:

We can pass the DEFAULT_OPTIONS as the first argument when parsing the arguments for constructing a model. The ArgHelper would handle merging the provided arguments with the defaults and we could take that logic out of a bunch of constructors.

We can merge this.options with additional options that are provided to a specific method. (For models like BodyPix where we are accepting options on methods like segment)

With my latest change they will get a warning if there are multiple options objects. I think that what you are suggesting is a good idea for a future PR. But for now this one should be good to go as-is!

# Conflicts: # src/BodyPix/index.js # src/FaceApi/index.js # src/Facemesh/index.js # src/Handpose/index.js

joeyklee · 2022-05-11T22:16:33Z

src/BodyPix/index.js

-
-    return callCallback(this.segmentWithPartsInternal(imgToSegment, segmentationOptions), callback);
-
+    return callCallback(this.segmentWithPartsInternal(image, options), callback);


Positive affirmation:

🧹 ✨ !!!

joeyklee · 2022-05-11T22:17:30Z

src/BodyPix/index.js

-  return callback ? instance : instance.ready;
+const bodyPix = (...inputs) => {
+  const args = handleArguments(...inputs);
+  const instance = new BodyPix(args.video, args.options || {}, args.callback);


Positive affirmation

🤩 digging this!

joeyklee · 2022-05-11T22:26:28Z

src/ImageClassifier/index.js

+  let model = string;
+  // TODO: I think we should delete this.
+  if (model.indexOf("http") === -1) {
+    model = model.toLowerCase();


This is a good note:

Maybe we keep this in for now, but let's do a follow up at some point reassessing the model name handling. I feel like we talked about these somewhere? 😂

Yes I created an issue #1339 but then I closed it because I interpreted the if statement backwards 🤦🏻‍♀️. I still think it should be handled a bit differently, but agreed that we should address it separately. I think I changed the one in ImageClassifier in #1362

joeyklee · 2022-05-11T22:28:37Z

src/NeuralNetwork/NeuralNetworkData.js

-      modelName = 'model';
-    }
+    const { string, callback } = handleArguments(nameOrCb, cb);
+    const modelName = string || 'model';


nonblocking comment:

handleArguements() feels like some kind of super hero magic -- "with great power comes great responsibility" 😂

joeyklee · 2022-05-11T22:42:54Z

src/SketchRNN/index.js


 // const PATH_START_LARGE = 'https://storage.googleapis.com/quickdraw-models/sketchRNN/large_models/';
 // const PATH_START_SMALL = 'https://storage.googleapis.com/quickdraw-models/sketchRNN/models/';
 // const PATH_END = '.gen.json';


 const DEFAULTS = {
+  // TODO: this doesn't look right... -Linda


nonblocking comment:

Oh weird! Hmm seems like we might add this to another follow up to resolve. 🙈

joeyklee · 2022-05-11T22:43:45Z

src/SketchRNN/index.js


    this.config = {
      temperature: 0.65,
      pixelFactor: 3.0,
+      // TODO: it doesn't make sense for these to be instance properties -Linda


nonblocking comment:

Agreed! Let's circle back around to this cleanup work.

joeyklee · 2022-05-11T22:50:08Z

src/utils/handleArguments.js

+ */
+export default function handleArguments(...args) {
+  return new ArgHelper(...args);
+};


Positive Affirmation:

Very clever + makes argument handling so much cleaner! Bravo! 🌟

joeyklee

Such a long needed overhaul and cleanup of this major part of ml5. Thanks so much! 🏅

Fix a bug that I introduced in #1322

lindapaiste added 4 commits March 7, 2022 21:43

Merge branch 'main' into cleanup/image-utilities

87fd122

Cleanup of image argument checking.

de9af76

Fixed bug re: this context in forEach

1c77583

Applied handleArguments to a few more models.

f1dc4d8

joeyklee reviewed Mar 10, 2022

View reviewed changes

src/utils/handleArguments.js Outdated Show resolved Hide resolved

joeyklee reviewed Mar 10, 2022

View reviewed changes

lindapaiste mentioned this pull request Apr 10, 2022

Preserve case in user-provided model URLs. #1339

Closed

lindapaiste added 2 commits May 8, 2022 14:02

Merge branch 'main' into cleanup/handle-arguments

98958eb

# Conflicts: # src/BodyPix/index.js # src/FaceApi/index.js # src/Facemesh/index.js # src/Handpose/index.js

Warn the user when there are duplicate arguments.

2f54164

lindapaiste requested a review from joeyklee May 8, 2022 19:25

joeyklee reviewed May 11, 2022

View reviewed changes

joeyklee approved these changes May 11, 2022

View reviewed changes

joeyklee added SEMVER/patch a version tag for a patch release change ready for release labels May 11, 2022

lindapaiste merged commit b6fd7eb into ml5js:main May 12, 2022

lindapaiste deleted the cleanup/handle-arguments branch May 12, 2022 17:58

lindapaiste mentioned this pull request May 14, 2022

Fix a bug that I introduced in #1322 #1385

Merged

joeyklee added a commit that referenced this pull request May 20, 2022

Merge pull request #1385 from lindapaiste/fix/options-undefined

e938aa3

Fix a bug that I introduced in #1322

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup 🧹 image argument checking #1322

Cleanup 🧹 image argument checking #1322

lindapaiste commented Mar 8, 2022

joeyklee Mar 10, 2022

lindapaiste Mar 12, 2022

lindapaiste May 8, 2022

joeyklee May 11, 2022

joeyklee May 11, 2022

joeyklee May 11, 2022

lindapaiste May 12, 2022

joeyklee May 11, 2022

joeyklee May 11, 2022

joeyklee May 11, 2022

joeyklee May 11, 2022

joeyklee left a comment


		return callCallback(this.segmentWithPartsInternal(imgToSegment, segmentationOptions), callback);

		return callCallback(this.segmentWithPartsInternal(image, options), callback);

Cleanup 🧹 image argument checking #1322

Cleanup 🧹 image argument checking #1322

Conversation

lindapaiste commented Mar 8, 2022

handleArguments

Utilities

Models

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyklee left a comment

Choose a reason for hiding this comment