Skip to content

Conversation

clnowacki
Copy link
Member

@clnowacki clnowacki commented Mar 21, 2024

@clnowacki clnowacki requested a review from jrobble March 21, 2024 15:23
@clnowacki clnowacki self-assigned this Mar 21, 2024
@jrobble jrobble requested a review from brosenberg42 March 25, 2024 15:46
This was referenced Mar 25, 2024
Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @brosenberg42 and @clnowacki)

a discussion (no related file):
Please add the generated HTML files, index files, etc. to this PR. Those are generated by running build_site.sh.

Also, please add a "Quality Selection Guide" entry to mkdocs.yml so a link appears on the left-hand navigation pane in ReadTheDocs.

Also, please format the .md file to the max chars per line.



docs/docs/Feed-Forward-Guide.md line 52 at r1 (raw file):

`FEED_FORWARD_TOP_QUALITY_COUNT` allows you to drop low quality detections from feed forward tracks. Setting the property to a value less than or equal to 0 has no effect. In that case all detections in the feed forward track will be processed.

When `FEED_FORWARD_TOP_QUALITY_COUNT` is set to a number greater than 0, say 5, then the top 5 highest quality detections in the feed forward track will be processed. Determination of quality is based on the job property `QUALITY_SELECTION_PROPERTY`, which defaults to `CONFIDENCE`, but may be set to a different detection property. [See the documentation in the "Quality Selection Guide"] If the track contains less than 5 detections then all of the detections in the track will be processed. If one or more detections have the same quality value, then the detection(s) with the lower frame index take precedence.

Please link to the Feed Forward Guide.


docs/docs/Quality-Selection-Guide.md line 8 at r1 (raw file):
Capitalize: "Workflow Manager"

For components that would rather use a different property as an indicator of quality, there was no means of indicating that to the workflow manager.

Let's not talk too much about what was. Let's save that discussion for the Release Notes. This guide should reflect the current behavior of the system.

Also, let's not mention release numbers since things are subject to change.


docs/docs/Quality-Selection-Guide.md line 20 at r1 (raw file):

Prior to release 9.0 of OpenMPF, the workflow manager was restricted to using detection confidence as its measure of quality. As mentioned above, for certain detection algorithms, there may be other detection properties that serve as a better measure of detection quality. With release 9.0, the user may now specify what property to use for all of the quality selection functionality mentioned above. 

For all OpenMPF components, there are now two properties that are used to control quality selection. The first is named `QUALITY_SELECTION_PROPERTY`. It is a string that defines the name of the property to use for quality selection. If this property is set to `CONFIDENCE`, then the `confidence` member of the OpenMPF `ImageLocation` object is used, just as it did in releases prior to 9.0. If it is set to any other string, then the workflow manager will search the `detection_properties` map in each `ImageLocation` for an entry with that key, and use the corresponding value as the detection quality. The value associated with this property must be an integer or floating point value, where higher values indicate higher quality.

I rephrased things to be more generic with "detections" and "tracks". That way it applies to image, video, audio, and generic jobs.

* Refactor Quality Selection Guide.

* Update Feed Forward Guide and C++ Streaming Component API.

* Add generated files.
Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 42 of 42 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @brosenberg42 and @clnowacki)

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

@clnowacki clnowacki merged commit 2d6ed79 into develop Mar 26, 2024
@clnowacki clnowacki deleted the feat/quality-selection branch March 26, 2024 16:42
Copy link
Member

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions


docs/docs/CPP-Streaming-Component-API.md line 180 at r2 (raw file):

Must return true when the component begins generating the first track for the current segment. After it returns true, the Component Executable will ignore the return value until the component begins processing the next segment.

If the `job_properties` map contained in the `MPFStreamingVideoJob` struct passed to the component constructor contains a `QUALITY_SELECTION_THRESHOLD` entry, then this function should only return true for a detection with a quality value that meets or exceeds that threshold. Refer to the [Quality Selection Guide](Quality-Selection-Guide/index.html). After the Component Executable invokes `EndSegment()` to retrieve the segment tracks, it will discard detections that are below the threshold. If all the detections in a track are below the threshold, then the entire track will be discarded.

Streaming only allows confidence for quality.


docs/docs/Quality-Selection-Guide.md line 24 at r2 (raw file):

One exception is when this property is set to `CONFIDENCE` and no `CONFIDENCE` property exists in the
`detection_properties` map. Then the `confidence` member of each detection and track is used instead.

We don't check for a "CONFIDENCE" property.

@jrobble
Copy link
Member

jrobble commented Mar 26, 2024

docs/docs/CPP-Streaming-Component-API.md line 180 at r2 (raw file):
Thanks for pointing this out. There are two parts to this:

  1. What the component developer should do within ProcessFrame(Mat ...)
  2. What the Component Executable does

While a developer could honor QUALITY_SELECTION_THRESHOLD within ProcessFrame(Mat ...), we would need to change how the Component Executable works to be compatible with that. We don't want to make those changes right now.

For now, let's change this to:

If the job_properties map contained in the MPFStreamingVideoJob struct passed to the component constructor contains a CONFIDENCE_THRESHOLD entry, then this function should only return true for a detection with a confidence value that meets or exceeds that threshold. After the Component Executable invokes EndSegment() to retrieve the segment tracks, it will discard detections that are below the threshold. If all the detections in a track are below the threshold, then the entire track will be discarded.

Note that in the future the C++ Streaming Component API should be updated to support QUALITY_THRESHOLD instead of CONFIDENCE_THRESHOLD.

@jrobble
Copy link
Member

jrobble commented Mar 26, 2024

docs/docs/Quality-Selection-Guide.md line 24 at r2 (raw file):
Thanks for pointing this out. I remember talking about this in the past, but we probably forgot about it. It is mentioned in the issue description:

By default, QUALITY_SELECTION_PROP is set to CONFIDENCE. The WFM will first check if a detection has a CONFIDENCE entry in the detection properties map, and if not, use the confidence field. (This is similar to how MARKUP_LABELS_NUMERIC_PROP_TO_SHOW works).

This is what Markup does:

    public static Optional<String> getLabel(Detection detection,
                                            String prefix,
                                            String textProp,
                                            int textLength,
                                            String numericProp) {
        String textStr = detection.getDetectionProperties().get(textProp);
        String numericStr = detection.getDetectionProperties().get(numericProp);
        if (numericStr == null && numericProp.equalsIgnoreCase("CONFIDENCE")) {
            numericStr = Float.toString(detection.getConfidence());
        }
        return getLabel(prefix, textStr, textLength, numericStr);
    }

For consistency, I think it would make sense to check the detection/track properties for CONFIDENCE before resorting to the confidence field.

@jrobble
Copy link
Member

jrobble commented Mar 26, 2024

docs/docs/Quality-Selection-Guide.md line 24 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Thanks for pointing this out. I remember talking about this in the past, but we probably forgot about it. It is mentioned in the issue description:

By default, QUALITY_SELECTION_PROP is set to CONFIDENCE. The WFM will first check if a detection has a CONFIDENCE entry in the detection properties map, and if not, use the confidence field. (This is similar to how MARKUP_LABELS_NUMERIC_PROP_TO_SHOW works).

This is what Markup does:

    public static Optional<String> getLabel(Detection detection,
                                            String prefix,
                                            String textProp,
                                            int textLength,
                                            String numericProp) {
        String textStr = detection.getDetectionProperties().get(textProp);
        String numericStr = detection.getDetectionProperties().get(numericProp);
        if (numericStr == null && numericProp.equalsIgnoreCase("CONFIDENCE")) {
            numericStr = Float.toString(detection.getConfidence());
        }
        return getLabel(prefix, textStr, textLength, numericStr);
    }

For consistency, I think it would make sense to check the detection/track properties for CONFIDENCE before resorting to the confidence field.

Thinking about this more, we may have decided against this because it was not possible for the WFM to check all of the detection property maps for all tracks at the same time. For example, we process the response for video segments separately. In that case if for some strange reason some of the detections / tracks have CONFIDENCE in their property map but other's don't then we could end up in a situation where sometimes we use the confidence field and sometimes we use the CONFIDENCE property.

Even if that is the case I think the same thing would happen right now in Markup since CONFIDENCE is checked on a detection-by-detection basis.

@jrobble
Copy link
Member

jrobble commented Mar 30, 2024

docs/docs/Quality-Selection-Guide.md line 24 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Thinking about this more, we may have decided against this because it was not possible for the WFM to check all of the detection property maps for all tracks at the same time. For example, we process the response for video segments separately. In that case if for some strange reason some of the detections / tracks have CONFIDENCE in their property map but other's don't then we could end up in a situation where sometimes we use the confidence field and sometimes we use the CONFIDENCE property.

Even if that is the case I think the same thing would happen right now in Markup since CONFIDENCE is checked on a detection-by-detection basis.

We decided to remove that behavior from Markup for consistency: openmpf/openmpf#1786

@jrobble
Copy link
Member

jrobble commented Mar 30, 2024

docs/docs/CPP-Streaming-Component-API.md line 180 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Thanks for pointing this out. There are two parts to this:

  1. What the component developer should do within ProcessFrame(Mat ...)
  2. What the Component Executable does

While a developer could honor QUALITY_SELECTION_THRESHOLD within ProcessFrame(Mat ...), we would need to change how the Component Executable works to be compatible with that. We don't want to make those changes right now.

For now, let's change this to:

If the job_properties map contained in the MPFStreamingVideoJob struct passed to the component constructor contains a CONFIDENCE_THRESHOLD entry, then this function should only return true for a detection with a confidence value that meets or exceeds that threshold. After the Component Executable invokes EndSegment() to retrieve the segment tracks, it will discard detections that are below the threshold. If all the detections in a track are below the threshold, then the entire track will be discarded.

Note that in the future the C++ Streaming Component API should be updated to support QUALITY_THRESHOLD instead of CONFIDENCE_THRESHOLD.

This change is addressed in #183

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

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants