-
-
Notifications
You must be signed in to change notification settings - Fork 25.5k
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
MAINT revert zero_division
introduced in 1.6
#30230
MAINT revert zero_division
introduced in 1.6
#30230
Conversation
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. Probably worth checking if doc build is okay too.
Please not so fast. Have we decided this in a closed call without open discussion? I know the release is approaching, but it feels rushed. |
@StefanieSenger the open discussion is here: #29048 (comment) |
This is a summary of a closed discussion. |
We didn't decide in a call to revert these changes. As a result of the discussion written in that issue, it became clear that we should revert these changes to avoid having to immediately deprecate something that we are going to release. So it makes sense to revert, so that we then put the new design in, w/o making the users go through a deprecation cycle. |
Yes it makes sense if we follow the path discussed in the private call. I can see it helps users not to go through a deprecation cycle and it might be the right decision overall. What I mean is that you don't give others the chance to voice their opinion. |
The comments on the issue and this PR is the place for people to give their opinions. However, there are enough people who don't want this to be released as it is right now, that it doesn't matter what others think. In scikit-learn, when we have some maintainers who are negative on a change, we have to find a better solution. That takes time and people are given time to raise their concerns. However, this is about preventing to push out a change in a release that some of us are not happy with. Therefore it needs to be pulled out of the release, and that doesn't need much of a discussion. However, how to replace it and how to move forward with it, needs discussion and very happy to have everyone's opinion about that. |
Okay, I can see your perspective on that and I agree that it is objectively the best way to do this. |
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.
@glemaitre There is a merge conflict. Otherwise LGTM
I solved the merge conflict. |
Related to #29048
This PR revert the introduction of
zero_division
such that we don't need to later deprecate it as discussed in #29048 (comment)In the bright side, since we are removing
zero_division
inaccuracy_score
, we can revisit if passing empty arrays into this function is actually a bug (ping @ogrisel). The test already shows that it is the only function that allows for such quirk feature.ping @adrinjalali @ogrisel