Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow matching empty frames in quality checks #8652
Allow matching empty frames in quality checks #8652
Changes from 27 commits
9605ac9
7bd4d87
bd12c4c
af6e862
1f53477
303375c
ccd3f8c
bc141e7
8826c0f
b2cbc05
51f757f
dc4667d
c8e5dc7
fe4d8c4
41f467a
418b333
01d5a07
f982371
f943343
129029e
d83a38f
03a5969
7e84f3a
3e9b7e1
975fdc0
6570d43
f8a8249
b42daa3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Initialize optional field
jaccard_index
inConfusionMatrix
.The
jaccard_index
field is optional but may cause issues if not properly initialized, especially when old serialized instances are used.Consider setting a default value in the constructor:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of these virtual annotations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, to break math and code less and get the expected values from
accuracy
,precision
, andrecall
here and in aggregated reports (job report, task report). For instance, if there are both empty and not empty frames, this helps to get correct metrics in an aggregated report. It's not totally nonsense, as an empty annotation can be considered a frame annotation by itself.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, but it seems like bad UX. You choose to match empty frames, and you inexplicably get fake totals. Moreover, if I understand the code correctly, with this change the totals no longer match the confusion matrix, which seems like it could cause more confusion and math errors down the line.
Is this solution really that much less disruptive than fudging the metric formulas to return 1 instead of 0?
This is a valid point, but you're not implementing that consistently. In your implementation, this "empty annotation" only appears when both GT and DS frames are empty. To do this consistently, you'd need to increase the GT count whenever the GT frame is empty, and the DS count whenever the DS frame is empty (and the valid count when both are).
You could also resolve the inconsistency between the confusion matrix and the totals by adding another row/column to the matrix specifically for these "empty annotations".
This would resolve the consistency issues, and slightly improve the UX issue, since you could see in the report where the extra annotation is coming from. But frankly, it still seems easier to me to just fudge the metrics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is really a useful thing to do here. Imagine a situation where there are 100 frames, frame 1 has 1 valid annotations and 2 total annotations, every other frame is empty.
In this case, with
match_empty_frames
off:With
match_empty_frames
on:Do you think this jump in total accuracy would be expected by the user?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to say about the expectations, to me the user just works in one of the modes - they either consider empty frames as annotated or not. But the second value seems to be more correct, so maybe something should be done for the case with the option disabled.