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

MNT deprecate ChangedBehaviorWarning and NonBLASDotWarning #17804

Merged
merged 3 commits into from Jul 5, 2020

Conversation

adrinjalali
Copy link
Member

Deprecates ChangedBehaviorWarning and NonBLASDotWarning since they're not used.

ping @NicolasHug @jeremiedbb I don't think we need any of them, do we?

@NicolasHug
Copy link
Member

Isn't there a chance we'll need ChangedBehaviorWarning in future? I don't remember what it was used for, maybe @amueller @jnothman @ogrisel @rth do?

@adrinjalali
Copy link
Member Author

My impression is that we eventually decided to use FutureWarning for all of them.

@jeremiedbb
Copy link
Member

jeremiedbb commented Jul 1, 2020

NonBLASDotWarning seems to have been used when numpy.dot had speed issues (version < 1.7). I think it has no utility anymore and can be removed safely.

You can remove this filter as well https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/utils/validation.py#L36

@adrinjalali
Copy link
Member Author

@jeremiedbb the filter is already removed in this PR, or at least I think it's removed. Don't you see it in the diff?

@jeremiedbb
Copy link
Member

jeremiedbb commented Jul 1, 2020

I specifically checked if you already did it and yet I missed it. The diff is too big :D

Copy link
Member

@rth rth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@amueller
Copy link
Member

amueller commented Jul 1, 2020

The ChangedBehaviorWarning was something we did after changing the behavior, so it's unlike future warning, but I also think it's not a good idea.

@adrinjalali adrinjalali merged commit 3a49e5f into scikit-learn:master Jul 5, 2020
5 checks passed
@adrinjalali adrinjalali deleted the NonBLASDotWarning branch July 5, 2020 20:32
@glemaitre
Copy link
Contributor

#17610 shows a use-case where ChangedBehaviorWarning would make sense. Basically, it would be linked to changing the default of a parameter for an experimental function/class. The reasoning of doing such was brought by @jnothman with who I agree:

I think forcing the user to always set as_frame is excessively conservative, and creates unnecessary as_frame= where the dataset would fail to load with as_frame=False (e.g. string columns).

I might think that we might need this warning at some point in HistGradientBoosting or IterativeImputer, for instance.

@adrinjalali
Copy link
Member Author

I'd say if we think we should give users warning about a change behavior in an experimental module, we're not thinking of it really as experimental and we should move it to stable, and deprecate old behavior as usual. I'd find it odd if we go down the path of warning users about change behavior in a module which is explicitly there for us to change things w/o a warning.

glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Jul 17, 2020
…arn#17804)

* deprecate ChangedBehaviorWarning and NonBLASDotWarning

* add whats_new entry
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
…arn#17804)

* deprecate ChangedBehaviorWarning and NonBLASDotWarning

* add whats_new entry
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.

None yet

6 participants