-
Notifications
You must be signed in to change notification settings - Fork 402
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Negative ignore_index
for the Accuracy metric
#362
Conversation
Hello @yassersouri! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2021-07-26 07:40:46 UTC |
Codecov Report
@@ Coverage Diff @@
## master #362 +/- ##
=====================================
- Coverage 95% 95% -0%
=====================================
Files 167 167
Lines 6906 6931 +25
=====================================
+ Hits 6565 6585 +20
- Misses 341 346 +5 |
@SkafteNicki All I need is a thumbs up or thumbs down on this. |
@yassersouri fine by me. The changes probably need to be done to the |
@yassersouri @SkafteNicki how is it going here? 🐰 |
58f1e14
to
caf7b0b
Compare
Hi again! Sorry for disappearing for a while. I will work on this pull request again.
Currently I think option 1 is messy as it requires making changes at multiple locations. I will try to implement version 2 and then gather your feedback. |
for more information, see https://pre-commit.ci
I just resolved the conflicts, please ignore the review request. |
for more information, see https://pre-commit.ci
This seems quite abundant; shall we close it? |
Having this option would be quite useful. |
@Borda @SkafteNicki Could you please have a look at this PR? I tried to fix some minor issues but I'm still not fully familiar with the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also LGTM, this should probably be extended to other classes when we try to refactor classification
As with the
CrossEntropy
loss in Pytorch, it is usual to have negative value forignore_index
.Currently the check in
_check_classifiction_inputs
(specifically_basic_input_validation
) does not allow a negative value for target.I have currently created some tests to show the problem.
If this is something that you think should be fixed, I can write a fix for it.
My proposal is to add
ignore_index
as an additional input to the_basic_input_validation
and slightly change the implementation.Please let me know if this is something that makes sense and you guys want it fixed.