Add cascade classifier operation by SamCarlberg · Pull Request #678 · WPIRoboticsProjects/GRIP

SamCarlberg · 2016-10-06T19:00:25Z

Also adds a new source for cascade classifiers

Goals

Code generation
File source for choosing XML configuration files
Add operation to extract ROIs from the image (can be a separate PR)
Add additional inputs for finer control over the classifier

Also adds a new socket input view for file selection

codecov-io · 2016-10-06T19:46:37Z

Current coverage is 53.92% (diff: 30.17%)

Merging #678 into master will decrease coverage by 0.55%

@@             master       #678   diff @@
==========================================
  Files           209        214     +5   
  Lines          6713       6882   +169   
  Methods           0          0          
  Messages          0          0          
  Branches        656        671    +15   
==========================================
+ Hits           3657       3711    +54   
- Misses         2886       3001   +115   
  Partials        170        170

Powered by Codecov. Last update 1d02ed1...844a91c

JLLeitschuh · 2016-10-06T20:54:37Z

ui/src/main/java/edu/wpi/grip/ui/preview/RectangleSocketPreviewView.java

+        this.infoLabel.setText("Found " + numRegions + " regions of interest");
+      });
+    }
+  }


This code seems eerily familiar, could this be refactored into the core (maybe behind an interface) with just a render method that takes a boolean showInputImage and also has a method like getDescriptionString?

I copy-pasted from the LinesReport preview, that's probably what you're thinking of. I'll pretty it up with a refactor later

Cool, if you move the render into core then it becomes incredibly easy for me to render as a web server too. Because then the code for creating the preview image will be in the core.

Done in 08e53d3

JLLeitschuh · 2016-10-07T15:56:12Z

core/src/main/java/edu/wpi/grip/core/sources/FileSource.java

+ * to the operations to resolve the file associated with the path.
+ */
+@XStreamAlias("grip:File")
+public class FileSource extends Source {


I would disagree with this pattern. I would make this a classifiers source.
Just like we don't expect steps to resolve paths for images that are loaded from the filesystem this too should behave the same.

JLLeitschuh · 2016-10-07T16:00:41Z

ui/src/main/java/edu/wpi/grip/ui/preview/LinesSocketPreviewView.java

  private final ImageConverter imageConverter = new ImageConverter();
  private final ImageView imageView = new ImageView();
  private final Label infoLabel = new Label();
-  private final Mat tmp = new Mat();


According to @ThomasJClark we cached the Mat in these classes because they were huge and were causing the memory usage of GRIP to grow at an incredibly fast rate and then the GC would kick in.
By caching the images in these temporary mats that we reused we were effectively re-using the same memory for each image rendered. If you can run a profiler and prove that this is not causing us to use more memory I'll allow this change, but you need to make sure of this.

This reverts commit 08e53d3. Revert "Fix MatUtil rename" This reverts commit 94c486b.

JLLeitschuh · 2016-10-08T02:26:35Z

core/src/main/java/edu/wpi/grip/core/util/MatUtils.java

-   *
-   * @return a mat with the data drawn on it
-   */
-  public static <T> Mat draw(Mat image,


I did like this solution, I just would have passed the tmp mat into it.

It's not relevant to this PR. It can get refactored later, especially since #680 has to get merged ASAP

SamCarlberg · 2016-10-10T17:46:31Z

It'd be nice to have a Size input socket to avoid having one or two New Size steps for each cascade classifier. @JLLeitschuh thoughts?

JLLeitschuh · 2016-10-10T19:15:42Z

core/src/main/java/edu/wpi/grip/core/operations/composite/CascadeClassifierOperation.java

+      OperationDescription.builder()
+          .name("Cascade Classifier")
+          .summary("Runs a cascade classifier on an image")
+          .icon(Icon.iconStream("opencv"))


Do you want a custom icon?

It's basically a one-to-one mapping of an OpenCV function, so it's okay as is

A nice icon for this feature would be nice.

It's basically a one-to-one mapping of an OpenCV function

All the other OpenCV operations use the OpenCV icon. So this will use the OpenCV icon, too.

JLLeitschuh · 2016-10-10T19:16:29Z

core/src/main/java/edu/wpi/grip/core/operations/composite/CascadeClassifierOperation.java

+      new SocketHint.Builder<>(Size.class)
+          .identifier("Max size")
+          .initialValue(new Size(0, 0))
+          .build();


Want to put these in the socket hints builder?

No need, the default already initializes to (0, 0). I'm just going to use that

JLLeitschuh · 2016-10-10T19:19:01Z

core/src/main/java/edu/wpi/grip/core/operations/composite/CascadeClassifierOperation.java

+      } catch (RuntimeException e) {
+        // Error with the config file, reset the classifier and throw an exception
+        classifier.load(lastFile);
+        throw new IllegalArgumentException("Invalid XML in configuration file", e);


The classifier should not be loaded from within the operation. The source should be the object instantiating the CascadeClassifier object

Think about this, if you ever want to add a HTTP source that creates a classifier then this won't work. Similarly, if someone sends one via network tables this won't work either.

That's a good idea, though I think that would have a negative impact on code generation because it would require users to manually instantiate the classifiers instead of having the pipeline handle everything

Cooooode gen!

Add missing import in generated Java code

JLLeitschuh · 2016-10-31T17:02:14Z

Is this ready to be merged?

SamCarlberg · 2016-10-31T17:40:28Z

Yes

JLLeitschuh · 2016-10-31T17:47:20Z

LGTM!

![Approved with PullApprove](https://img.shields.io/badge/GRIP Merge Approvers-approved-brightgreen.svg)

Add cascade classfier operation

96fa217

Also adds a new socket input view for file selection

JLLeitschuh reviewed Oct 6, 2016

View reviewed changes

Add FileSource, remove file sockets

4afbf20

SamCarlberg force-pushed the feat/classifiers branch from 954cbef to 4afbf20 Compare October 7, 2016 13:31

SamCarlberg added 3 commits October 7, 2016 10:22

Move Mat drawing to a utility class

08e53d3

Fix MatUtil rename

94c486b

Add code generation

7ac96e1

JLLeitschuh reviewed Oct 7, 2016

View reviewed changes

SamCarlberg added 3 commits October 7, 2016 13:49

Revert MatUtil commits

bc0d2e7

This reverts commit 08e53d3. Revert "Fix MatUtil rename" This reverts commit 94c486b.

Change generic "FileSource" to specialized "ClassifierSource"

00a97d8

Add scale factor, min neighbors, and min/max size to cascade classifier

a0643eb

JLLeitschuh reviewed Oct 8, 2016

View reviewed changes

SamCarlberg changed the title ~~Add cascade classfier operation~~ Add cascade classifier operation Oct 10, 2016

Update code generation

d196172

JLLeitschuh reviewed Oct 10, 2016

View reviewed changes

SamCarlberg added 3 commits October 10, 2016 15:25

Use pre-existing hints for size sockets

50a399d

Fix a bug when trying to fall back to the previous file

e6482bb

Make classifier source supply a classifier object

a4a89d8

Add missing import in generated Java code

SamCarlberg added this to the v1.5.0 milestone Oct 24, 2016

Throw IOException if classifier file is invalid

844a91c

SamCarlberg force-pushed the feat/classifiers branch from 422f92e to 844a91c Compare October 25, 2016 04:17

JLLeitschuh approved these changes Oct 31, 2016

View reviewed changes

SamCarlberg merged commit 0d8f24f into WPIRoboticsProjects:master Oct 31, 2016

Comments

Conversation

SamCarlberg commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goals

Uh oh!

codecov-io commented Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 53.92% (diff: 30.17%)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamCarlberg commented Oct 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SamCarlberg Oct 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JLLeitschuh commented Oct 31, 2016

Uh oh!

SamCarlberg commented Oct 31, 2016

Uh oh!

JLLeitschuh commented Oct 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SamCarlberg commented Oct 6, 2016 •

edited

Loading

codecov-io commented Oct 6, 2016 •

edited

Loading

SamCarlberg Oct 10, 2016 •

edited

Loading

JLLeitschuh commented Oct 31, 2016 •

edited

Loading