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

Improved SCRFD decoding. #21

Merged
merged 4 commits into from
Aug 19, 2024
Merged

Improved SCRFD decoding. #21

merged 4 commits into from
Aug 19, 2024

Conversation

kkeroo
Copy link
Collaborator

@kkeroo kkeroo commented Aug 14, 2024

This PR implements better SCRFD decoding with keypoints. It follows the official decoding procedure. Decoding is moved to the utils files. Some small validation changes are made in detections creators and message class @jkbmrz so it accepts also keypoints as list and not only tuple. This is for easier conversion from numpy array (going only with np_array.tolist(). This shouldn't break YuNet parser.

Some additional parameters are added to the parser e.g. input_size, feature_strides etc. because if we add different scrfd model (lighter, heavier) in the future we only adjust those params.

Note that NMS function is added here. We should address using one NMS for all parsers where posssible in the separate task.

There is also support for RVC4 decoding so it works for both platforms.

@kkeroo kkeroo requested a review from jkbmrz August 14, 2024 09:37
@kkeroo kkeroo self-assigned this Aug 14, 2024
Copy link
Collaborator

@klemen1999 klemen1999 left a comment

Choose a reason for hiding this comment

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

Left some comments, otherwise LGTM.
I agree that we should also make general NMS (separate PR) that can be reused accross all parsers.

@@ -98,7 +98,7 @@ def create_detection_message(
raise ValueError(f"keypoints should be list, got {type(keypoints)}.")
for pointcloud in keypoints:
for point in pointcloud:
if not isinstance(point, Tuple):
if not isinstance(point, Tuple) and not isinstance(point, List):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: Is there a better name for a variable than pointcloud. As I understand keypoints are a list of (n,2) keypoints for specific object? Maybe:

for object_keypoints in keypoints:
    for point in object_keypoints:
        ....

Feel free to suggest other names though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm yeah good point. I agree with the proposed naming. Added in a6b3b02.

score_threshold=self.score_threshold,
nms_threshold=self.nms_threshold,
)
detection_msg = create_detection_message(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In line to PR #19 we should also add timestamp to the message.
Should we make this part of the create_detection_message() function already? (Can be a separate PR)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, added it when you were reviewing. Hmm I was thinking that it would be best to add it to the creator functions but we need output to get the timestamp. Since it is only one line of code we leave it here? Because otherwise we need to propagate output to every creator function.

@kkeroo kkeroo requested a review from klemen1999 August 14, 2024 11:00
Copy link
Collaborator

@jkbmrz jkbmrz left a comment

Choose a reason for hiding this comment

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

LGTM

@kkeroo kkeroo merged commit ceb642c into main Aug 19, 2024
1 check passed
@kkeroo kkeroo deleted the improvement/scrfd branch August 19, 2024 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants