Skip to content

Conversation

clnowacki
Copy link
Member

@clnowacki clnowacki commented Feb 21, 2024

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.

Reviewed 8 of 13 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @clnowacki and @jrobble)


cpp/OcvDnnDetection/OcvDnnDetection.cpp line 674 at r2 (raw file):

    std::string quality_property = GetProperty(props, "QUALITY_SELECTION_PROPERTY", "CONFIDENCE");
    if (quality_property != "CONFIDENCE") {
        throw std::runtime_error("Unrecognized quality selection property \"" + quality_property + "\". Only CONFIDENCE is supported for quality selection.");

We should be throwing MPFInvalidPropertyException in this case. For example:

throw MPFInvalidPropertyException(
    "QUALITY_SELECTION_PROPERTY",
    "Unrecognized quality selection property \"" 
        + quality_property + "\". Only CONFIDENCE is supported for quality selection."
);

cpp/OcvDnnDetection/plugin-files/descriptor/descriptor.json line 140 at r2 (raw file):

          "description": "Minimum value of the QUALITY_SELECTION_PROPERTY needed to keep detection and start a track.",
          "type": "DOUBLE",
          "defaultValue": "detection.quality.selection.threshold"

This should either be:
"defaultValue": "0"
or
"propertiesKey": "detection.quality.selection.threshold"


cpp/OcvYoloDetection/Config.cpp line 96 at r2 (raw file):

            std::string quality_property = GetProperty(jobProps, "QUALITY_SELECTION_PROPERTY", "CONFIDENCE");
            if (quality_property != "CONFIDENCE") {
                throw std::runtime_error("Unsupported quality selection property \"" + quality_property + "\". Only CONFIDENCE is supported for quality selection.");

This should throw MPFInvalidPropertyException.


python/EastTextDetection/east_component/east_component.py line 60 at r2 (raw file):

        quality_property = props.get('QUALITY_SELECTION_PROPERTY', 'CONFIDENCE')
        if quality_property != "CONFIDENCE":
            raise ValueError('Unsupported quality selection property \"' + quality_property + '\". Only CONFIDENCE is supported.')

This should raise mpf.DetectionError.INVALID_PROPERTY.exception('Unsupported quality selection ...')

Copy link
Member Author

@clnowacki clnowacki 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: 9 of 14 files reviewed, 4 unresolved discussions (waiting on @brosenberg42 and @jrobble)


cpp/OcvDnnDetection/OcvDnnDetection.cpp line 674 at r2 (raw file):

Previously, brosenberg42 wrote…

We should be throwing MPFInvalidPropertyException in this case. For example:

throw MPFInvalidPropertyException(
    "QUALITY_SELECTION_PROPERTY",
    "Unrecognized quality selection property \"" 
        + quality_property + "\". Only CONFIDENCE is supported for quality selection."
);

Done.


cpp/OcvDnnDetection/plugin-files/descriptor/descriptor.json line 140 at r2 (raw file):

Previously, brosenberg42 wrote…

This should either be:
"defaultValue": "0"
or
"propertiesKey": "detection.quality.selection.threshold"

Done.


cpp/OcvYoloDetection/Config.cpp line 96 at r2 (raw file):

Previously, brosenberg42 wrote…

This should throw MPFInvalidPropertyException.

Done.


python/EastTextDetection/east_component/east_component.py line 60 at r2 (raw file):

Previously, brosenberg42 wrote…

This should raise mpf.DetectionError.INVALID_PROPERTY.exception('Unsupported quality selection ...')

Done.

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.

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 12 of 14 files reviewed, 1 unresolved discussion (waiting on @clnowacki and @jrobble)


cpp/OcvDnnDetection/OcvDnnDetection.cpp line 674 at r2 (raw file):
In the example in my comment, I pass "QUALITY_SELECTION_PROPERTY" as the first argument. The first argument to MPFInvalidPropertyException is the property name and the second is the message. MPFInvalidPropertyException creates the .what() message with: "The " + propertyName + " job property contained an invalid value. " + reason.

Currently, the .what() message is:

The Unrecognized quality selection property "<quality_property>". Only CONFIDENCE is supported for quality selection. job property contained an invalid value.

This is only an issue for the C++ code. The way you did in Python is correct.


cpp/OcvYoloDetection/Config.cpp line 96 at r2 (raw file):

Previously, clnowacki wrote…

Done.

The first parameter to MPFInvalidPropertyException should be the name of the property.

Copy link
Member Author

@clnowacki clnowacki 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: 12 of 14 files reviewed, 1 unresolved discussion (waiting on @brosenberg42 and @jrobble)


cpp/OcvYoloDetection/Config.cpp line 96 at r2 (raw file):

Previously, brosenberg42 wrote…

The first parameter to MPFInvalidPropertyException should be the name of the property.

Done.

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.

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @jrobble)

@clnowacki clnowacki merged commit b8e0bdd into develop Mar 11, 2024
@clnowacki clnowacki deleted the feat/quality-selection branch March 11, 2024 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants