Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix interaction between top_k and ignore_index #1589

Merged
merged 7 commits into from Mar 6, 2023

Conversation

SkafteNicki
Copy link
Member

What does this PR do?

Fixes #1556
Fixes an cornercase when top_k>1 and ignore_index!=None. The case was not tested, but test have been added now.

Before submitting
  • Was this discussed/agreed via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
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 馃檭

@SkafteNicki SkafteNicki added the bug / fix Something isn't working label Mar 5, 2023
@SkafteNicki SkafteNicki added this to the v0.12 milestone Mar 5, 2023
@SkafteNicki SkafteNicki added this to In progress in Classification refactor via automation Mar 5, 2023
@mergify mergify bot added the has conflicts label Mar 5, 2023
@mergify mergify bot removed the has conflicts label Mar 6, 2023
@Borda Borda enabled auto-merge (squash) March 6, 2023 07:02
@mergify mergify bot requested a review from a team March 6, 2023 07:02
@codecov
Copy link

codecov bot commented Mar 6, 2023

Codecov Report

Merging #1589 (413602f) into master (a5df1c6) will decrease coverage by 37%.
The diff coverage is 100%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #1589     +/-   ##
========================================
- Coverage      88%     51%    -37%     
========================================
  Files         225     225             
  Lines       11904   11905      +1     
========================================
- Hits        10460    6044   -4416     
- Misses       1444    5861   +4417     

@mergify mergify bot added the ready label Mar 6, 2023
@Borda Borda disabled auto-merge March 6, 2023 14:23
@Borda Borda changed the title Bugfix: fix interaction between top_k and ignore_index fix interaction between top_k and ignore_index Mar 6, 2023
@Borda Borda enabled auto-merge (squash) March 6, 2023 14:24
@Borda Borda requested a review from stancld March 6, 2023 14:54
@Borda Borda merged commit 0d28f26 into master Mar 6, 2023
Classification refactor automation moved this from In progress to Done Mar 6, 2023
@Borda Borda deleted the bugfix/top_k_ignore_index branch March 6, 2023 17:50
Borda pushed a commit that referenced this pull request Mar 10, 2023
* fix code
* changelog
* fix tests

(cherry picked from commit 0d28f26)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug / fix Something isn't working ready
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

IndexError when computing multiclass accuracy with topk and ignore_index
4 participants