hotfix on classification metrics

[Diuven]

What does this PR do?

Fixes #2862

Change the stat_scores_multiple_classes not to change the input tensors, by removing in-place operations.

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 🙃

[awaelchli]

good catch! I wonder if other metrics have this issue too.

[Diuven]

good catch! I wonder if other metrics have this issue too.

I think this is all we need to fix. The major changes were all in the stat_scores_multiple function and all other metric functions are based on this. Such functions would have had a same problem, but fixing this will fix them all.

Though if there are anything more I missed or mistaken, pls tell. 😄

[codecov]

Codecov Report

Merging #2878 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2878   +/-   ##
======================================
  Coverage      90%     90%           
======================================
  Files          79      79           
  Lines        7259    7226   -33     
======================================
- Hits         6524    6519    -5     
+ Misses        735     707   -28     

[Borda]

can we a test to show that the master fails and this fixes it?

[Borda]

Again, we were waiting for tests to be added...
@Diuven mind send another PR with test =)

[awaelchli]

I would usually also like to see a test. For corrrectness we would need such a test for every metric, not just this one, but then how do we generalize this test for every metric? They are all different.

[Diuven]

Again, we were waiting for tests to be added...
@Diuven mind send another PR with test =)

TL,DR; I added the tests for my changes, and I'm not sure about whether I should add more or not. If you want more tests, you might want to add tests for stat_scores_multiple_classes.

I agree on that we need more tests on this part of the code. In fact, I did add some tests for stat_scores_multiple_classes function, as I added a new parameter reduction in it. You can check this diff.

If you want yo verify my changes, you might want to test the stat_scores_multiple_classes very carefully, rather than adding more cases on other metrics. They all rely on the stat_scores_multiple_classes function, and all I changed is that. (Of course, adding more test for other metrics never hurt, but you might not need it.)

However, I think tests cannot encompass every possible cases and expectation checks. This in-place bug would be need a lot of test cases and checks, for each functions. Though that's doable, I don't think we can we can verify such cases - not only things like in-place ops - effectively. Considering and catching such bugs before merging is ideal and great, but I think some of them can just get through sometimes, and we can catch them later on when we have a visible issue.

Moreover, as I'm not a core contributor of PL, I'm not sure about the expected behavior of the edge cases. For example, justusschock mentioned about the expected results about unmatching num_classes before in this conversation. I didn't considered such cases in my speedup PR, so I missed that part and the tests (by justusschock probably) caught that case for me before the PR.
The point is that as I'm not the one who decide how it should work in such unusual cases, it's very hard for me to add such cases. I can mimic and extend the existing tests when I augment the code, but can't add more than that.

Even though, I'm happy to discuss in depth about this part of the project. If you may, please open up such conversation and mention me! =)