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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAINT revert zero_division introduced in 1.6 #30230

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

glemaitre
Copy link
Member

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 in accuracy_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

Copy link

github-actions bot commented Nov 6, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: d4691ff. Link to the linter CI: here

Copy link
Member

@adrinjalali adrinjalali left a 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.

@StefanieSenger
Copy link
Contributor

StefanieSenger commented Nov 7, 2024

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.

@adrinjalali
Copy link
Member

@StefanieSenger the open discussion is here: #29048 (comment)

@StefanieSenger
Copy link
Contributor

@StefanieSenger the open discussion is here: #29048 (comment)

This is a summary of a closed discussion.

@adrinjalali
Copy link
Member

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.

@StefanieSenger
Copy link
Contributor

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.

@adrinjalali
Copy link
Member

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.

@StefanieSenger
Copy link
Contributor

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.

@glemaitre glemaitre added this to the 1.6 milestone Nov 7, 2024
Copy link
Member

@thomasjpfan thomasjpfan left a 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

@glemaitre
Copy link
Member Author

I solved the merge conflict.

@thomasjpfan thomasjpfan enabled auto-merge (squash) November 12, 2024 16:19
@thomasjpfan thomasjpfan merged commit 3b22ec0 into scikit-learn:main Nov 12, 2024
28 checks passed
jeremiedbb pushed a commit to glemaitre/scikit-learn that referenced this pull request Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants