Group metrics by labels - #245
Conversation
Sorry, something went wrong.
sasha-scale
left a comment
There was a problem hiding this comment.
Overall looks pretty clean to me - all comments are nits. My biggest concern with this change is that I don't want to overcomplicate the case where a user doesn't care about class specific results and just wants to aggregate results. As such, I wonder if it would be beneficial to change the interface of the aggregate_results method to return a scalar and to have all metric classes implement it?
Sorry, something went wrong.
| value = f1_score(gt, predicted, average=self.f1_method) | ||
| return ScalarResult(value) | ||
| results = {} | ||
| results["macro"] = f1_score(gt, predicted, average="macro") |
There was a problem hiding this comment.
:nit: avoid hardcoded strings and instead make them constants
Sorry, something went wrong.
| def __init__( | ||
| self, | ||
| enforce_label_match: bool = False, | ||
| enforce_label_match: bool = True, |
There was a problem hiding this comment.
:nit: please adjust comment below
Sorry, something went wrong.
| ScalarResult(109.0 / 300, 3), | ||
| {"enforce_label_match": False}, | ||
| ), | ||
| # ( |
There was a problem hiding this comment.
remove commented code before merging
Sorry, something went wrong.
There was a problem hiding this comment.
Of course 🙂
Sorry, something went wrong.
Yeah, that is a valid concern. It definitely warrants further concern to take a look at the interface of how and where we choose to group_by and aggregate. I'll try to figure out a suggestion today. |
Sorry, something went wrong.
| # TODO(gunnar): Enforce label match -> Why is that a parameter? Should we generally allow IOU matches | ||
| # between different labels?!? |
There was a problem hiding this comment.
In general we should have an option to allow this. E.g. you need to compute matches across the classes for the confusion matrix.
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Context:
We want to support class-specific results from our metrics. As a part of that we're essentially opening up the interface from the metrics to return multiple values with a label per value. For our standard functions we will return a value per label for the polygon metrics.
Before:
We'd get a single number per DatasetItem and then a single aggregate_score per evaluation.
After:
We get a {"key_1": number, ... , "key_N": number} per DatasetItem and then another {"another_key_1": number, ..., "another_key_N": number } from aggregate_score.
As a part of this we also add extra_info which is a string -> string dictionary which you can add to each dataset item in the metric and error field which we use if evaluation of a single DatasetItem fails. That way we don't fail the whole evaluation.
This PR
This changes the metrics to allow returning multiple values per metrics and changes the default metrics to return results grouped by label.
This also adds an extra method on results that allow us to pass more data than floats to the frontend via extra_info, example is that we send the weight of each dataset item with the ScalarResult right now.