-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
Is it intention to drop check_estimator_sparse_data
in 1.5?
#28966
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
Comments
Thanks for the feedback on the RC @koaning ! This check was split into 2 different checks, one for sparse matrices and one for sparse arrays, see #27576. It's kind of a grey area because all these checks are not really public so we may change them without deprecation. I think it can't hurt to at least mention it in the changelog. Note that there's also ongoing work to refactor these checks to expose a third party developer API for which these considerations will be better defined. |
When is something in the public API? I assumed it was public enough because it publicly accessible (no underscore prefix in the function name). I think we'll be fine with making the switch on the sklego side but there may certainly be other 3rd party packages that use this function because you kind of need to use these testing functions if you want to guarantee that you are compatible to the sklearn API. |
Agree with Jeremie about it being a gray area. I think we should follow the normal deprecation policy if we change something related to testing estimators that is directly mentioned in the documentation https://scikit-learn.org/dev/developers/develop.html#rolling-your-own-estimator (Maybe another thing to remember is when we change the checks so that estimators that used to pass suddenly fail) However the individual checks, what they are called and what they do is probably an "implementation detail". So I think we should make sure that we steer people towards using things that we want them to use, and away from the other stuff. |
to me, the official public API is what is officially documented, i.e. in |
Fair enough, the tests in scikit-lego are pretty old (>6 years) so it could be perfectly fine/normal for us to revamp it. The only small warning is that there may be a fair amount of other plugin packages who are going to notice, but if this is mentioned in the release notes that should suffice. |
We certainly need to have a public set of tests for third party developers. It's along the lines of what @glemaitre has been working on. |
Describe the bug
While running the 1.5.0 release candidate, my collaborator @FBruzzesi on scikit-lego ran our testing suite and noticed something breaking. Here's the error message:
Figured I'd ping and check, did a function get renamed? If so, it feels breaking and I can't recall a warning (but I may be mistaken, please tell me if I missed that). This is mostly just a ping issue so folks are aware, feel free to close if this feels like a false alarm.
Steps/Code to Reproduce
Expected Results
No error is thrown.
Actual Results
Versions
The text was updated successfully, but these errors were encountered: