-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
TST allow setting per test settings for estimators #29820
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
Conversation
@ogrisel @glemaitre this is the per-test setting PR. |
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.
Shall we offer a semi-public dev API to register custom params for third party estimator developers?
If so, it should probably be documented somewhere (along with the estimator check API) in the user guide, probably at one of the following:
By the way I can also review this PR as is (as an internal refactoring) without thinking about third party developer API first and defer the third party estimator API to a later PR. |
@ogrisel I plan to write a new page for "how to write your estimator" anyway, but a bit later. These will certainly find their way there. I also plan to expose this functionality to third party devs, but first prefer to be mostly done with test refactoring, since during this phase we're gonna add/remove/rename a lot of tests. But I added a note in the code to remember this point. |
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 then. Maybe a follow-up could be to allow registering custom test data generators per-estimators (if we have a need for that).
Apparently it used to be the case for RANSACRegressor
but it isn't anymore based on this diff?
Yeah I was happy that, that part of the code could be removed, but when we get to supporting non-numerical data for estimators, that test-generator-registration is gonna be necessary anyway. So it'll come. |
Maybe you might want to see if Alternatively we can do it in a follow-up. |
I see. That's easy to include, but needs a tiny bit of restructuring. Happy to do that, but rather keep these PRs as small as it gets so that we move forward fast. I can work on that once we merge this PR. |
@ogrisel note that the test you mention has two issues:
This seems like something we should tackle separate from this PR. |
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.
allow multiple settings for each test...