feat: add class_names support to Precision, Recall, FBeta metrics - #3732
feat: add class_names support to Precision, Recall, FBeta metrics#3732aniketpandey05 wants to merge 15 commits into
Conversation
|
ok so like wont this code break There can be more metrics that can benefit from this change. I am not sure though. |
|
@aaishwarymishra would raising errors for those condition a better option or should we add full class_name support to fbeta |
|
I am not sure, adding support for the class names in |
|
Could we please clarify |
If we can compute F1 score per class than we should propagate the dict structure until the output of the F1 otherwise, take |
|
Hey @vfdev-5 , would adding a helper in metric.py to handle dict arithmetic in all operator overloads so the dict propagates through to the Fbeta output be a good approach? Also when average=True and class_names is set, should Fbeta return a scalar float or propagate the dict as is? |
I do not know, what would you suggest? Let's try to make it the most simple possible and intuitively clear. |
|
hey @vfdev-5 I think Fbeta should return a per-class dict if class_names is set, regardless of average. |
|
@rogueslasher ok, let's implement that this way and see if this works as expected |
|
Hey @vfdev-5, would adding a _dict_aware_op helper in metric.py and updating all operator overloads to use it for element-wise dict arithmetic, and skipping average in fbeta.py when class_names is set be a good idea? It touches all operator overloads in metric.py which affects all metrics. i am worried since it touches all operators overloads |
|
@aaishwarymishra ,would love your help in this can you look into it more please |
|
@rogueslasher well , I am not sure to be honest i wont touch the metric operators as it kind of affect every metric, for a case which can be only relevant for very few metrics, I think we should modify |
61e7680 to
7ea2eb5
Compare
|
@aaishwarymishra the header rules and other three preview checks are taking insane amount of time . is there any issue |
|
Documentation issue :) |
9c71695 to
817f33b
Compare
There was a problem hiding this comment.
Also there is no early check for if average is True and also the class name has been given
3a93092 to
7e12121
Compare
|
Hi @aaishwarymishra, thanks for the review! I've updated the implementation: |
|
Also try to reslove previous comments it gets hard to read the code :) |
aebabb1 to
7b8ce31
Compare
|
@aaishwarymishra i think the ci failure are unrelated to the changed files |
|
@vfdev-5 looks good to me, you can check it too and merge :) |
vfdev-5
left a comment
There was a problem hiding this comment.
I haven't yet checked the code itself but docstring needs some updates.
@aaishwarymishra resolve all open comments if they were addressed.
| device: specifies which device updates are accumulated on. Setting the metric's | ||
| device to be the same as your ``update`` arguments ensures the ``update`` method is non-blocking. By | ||
| default, CPU. | ||
| class_names: list of class name strings used to label per-class output. Default: ``None``. |
There was a problem hiding this comment.
Docstring should also contain .. version added tag for new args
aaishwarymishra
left a comment
There was a problem hiding this comment.
@aniketpandey05 can you update docstrings too :)
| default, CPU. | ||
| class_names: list of class name strings used to label per-class output. Default: ``None``. | ||
|
|
||
| .. versionadded:: 0.6.0 |
There was a problem hiding this comment.
This is a wrong tag, check Contributing guide and also the indent size.
| target_class_names = class_names | ||
| if target_class_names is None and precision is not None: | ||
| target_class_names = precision._class_names | ||
| if target_class_names is None and recall is not None: | ||
| target_class_names = recall._class_names |
There was a problem hiding this comment.
| target_class_names = class_names | |
| if target_class_names is None and precision is not None: | |
| target_class_names = precision._class_names | |
| if target_class_names is None and recall is not None: | |
| target_class_names = recall._class_names | |
| if class_names is None and precision is not None: | |
| class_names = precision._class_names | |
| if class_names is None and recall is not None: | |
| class_names = recall._class_names |
| output_transform=(lambda x: x) if output_transform is None else output_transform, | ||
| average=False, | ||
| device=cast(str | torch.device, recall._device if recall else device), | ||
| class_names=target_class_names, |
There was a problem hiding this comment.
| class_names=target_class_names, | |
| class_names=class_names, |
| output_transform=(lambda x: x) if output_transform is None else output_transform, | ||
| average=False, | ||
| device=cast(str | torch.device, precision._device if precision else device), | ||
| class_names=target_class_names, |
There was a problem hiding this comment.
| class_names=target_class_names, | |
| class_names=class_names, |
| if any(m._average for m in active_metrics): | ||
| raise ValueError("Input precision and recall metrics should have average=False") |
There was a problem hiding this comment.
This new error message is not precise enough. We should explicitly tell which metric exactly should have average false and not give a generic message
|
|
||
| @sync_all_reduce("_numerator", "_denominator") | ||
| def compute(self) -> torch.Tensor | float: | ||
| def compute(self) -> torch.Tensor | float | dict: |
There was a problem hiding this comment.
Can you make dict type hint more detailed: dict[str, <whatever type is here>]
| name to its metric value instead of a tensor. Must match the number of classes inferred | ||
| from the data. Default: ``None``. | ||
|
|
||
| .. versionadded:: 0.6.0 |
There was a problem hiding this comment.
this is a wrong tag, it should not be added here.
| name to its metric value instead of a tensor. Must match the number of classes inferred | ||
| from the data. Default: ``None``. | ||
|
|
||
| .. versionadded:: 0.6.0 |
There was a problem hiding this comment.
| .. versionadded:: 0.6.0 |
| Fbeta(0.0) | ||
|
|
||
| with pytest.raises(ValueError, match=r"Input precision metric should have average=False"): | ||
| with pytest.raises(ValueError, match=r"Input precision and recall metrics should have average=False"): |
There was a problem hiding this comment.
Keep this test as it was and update the code
| Fbeta(1.0, precision=p) | ||
|
|
||
| with pytest.raises(ValueError, match=r"Input recall metric should have average=False"): | ||
| with pytest.raises(ValueError, match=r"Input precision and recall metrics should have average=False"): |
| with pytest.raises(ValueError, match="Input precision and recall metrics should have average=False"): | ||
| Fbeta(beta=1.0, average=False, precision=p_avg) | ||
|
|
||
| # Correct computation passing class_names directly to Fbeta |
There was a problem hiding this comment.
I prefer that these test cases were added to existing tests as new parametrizations.
Fixes #1466
Description:
Adds an optional
class_namesparameter to_BasePrecisionRecall, allowingcompute()to return a labeleddictinstead of an unnamed tensor whenaverage=Falseoraverage=None. Useful for per-class metric tracking where knowing which score belongs to which class matters for logging and visualization.Check list: