Corrected f_beta computation

[abhinavg97]

Added METRIC_EPS in the denominator of computation to avoid nan values in f_beta score.

What does this PR do?

Will give the correct f_beta score and prevent outputting nan values for multilabel classification with macro.

Fixes #4187

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃 👍

[pep8speaks]

Hello @abhinavg97! Thanks for updating this PR.

• In the file tests/metrics/classification/test_f_beta.py:

Line 91:121: E501 line too long (121 > 120 characters)

Comment last updated at 2020-10-21 08:07:39 UTC

[codecov]

Codecov Report

Merging #4183 into master will decrease coverage by 1%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #4183    +/-   ##
=======================================
- Coverage      90%     89%    -1%     
=======================================
  Files         103     103            
  Lines        7863    8313   +450     
=======================================
+ Hits         7064    7403   +339     
- Misses        799     910   +111     

[Borda]

I think that this offset epsilon adding to all is not good, we shall check it there are zeros and then add it just there, well if know that some are rore just retunt the output...
@justusschock @SkafteNicki

[SkafteNicki]

We have this function (still used in functional) that took care of this:
https://github.com/PyTorchLightning/pytorch-lightning/blob/130de22fd75253330320473d8081fd60698c3d64/pytorch_lightning/metrics/functional/reduction.py#L40-L78
We should probably reuse it in the new class api

[SkafteNicki]

@abhinavg97 is it correct that this happens when both recall and precision is 0 for a particular class?
Could we add a test case for this?

[abhinavg97]

Yes, that's right.

Here is an example:

predictions = torch.Tensor([[0,0],[0,1]])
labels = torch.Tensor([[0,1],[1,0]])

f_beta = Fbeta(num_classes=len(t[0]), average='macro', multilabel=True)

f_beta(predictions, labels)

> torch(nan)

Expected: torch(0)

Yeah I think we can add a test for this. Is it okay if I add the above case as a test?

[Borda]

can we add a test to verify that these changes fix the actual master...?

[SkafteNicki]

@abhinavg97 can you add the following testcase to tests/metrics/classification/inputs and afterwards add to the corresponding parametrization of the f-beta tests:

# Generate edge multilabel edge case, where nothing matches (scores are undefined)
__temp_preds = torch.randint(high=2, size=(NUM_BATCHES, BATCH_SIZE, NUM_CLASSES))
__temp_target = abs(__temp_preds - 1)

_multilabel_inputs_no_match = Input(
    preds=__temp_preds,
    target=__temp_target
)

[abhinavg97]

@abhinavg97 can you add the following testcase to tests/metrics/classification/inputs and afterwards add to the corresponding parametrization of the f-beta tests:

# Generate edge multilabel edge case, where nothing matches (scores are undefined)
__temp_preds = torch.randint(high=2, size=(NUM_BATCHES, BATCH_SIZE, NUM_CLASSES))
__temp_target = abs(__temp_preds - 1)

_multilabel_inputs_no_match = Input(
    preds=__temp_preds,
    target=__temp_target
)

Yes sure, will do. Thanks for the pointer to the input location :)

[SkafteNicki]

LGTM

[tchaton]

Great PR ! Thanks !

[Borda]

do we really want to keep our metric systematically unprecise by adding some offset?

[SkafteNicki]

we can move towards class_reduce function which explicit handles nans, when we unify the class based metrics and functional metrics.

[abhinavg97]

yeah, makes sense. Just checked and the tests pass without METRIC_EPS. Pushing the update now.