Skip to content

Add special case handling for default engine #25549

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

Merged

Conversation

betatim
Copy link
Member

@betatim betatim commented Feb 6, 2023

This fixes the problem that if you did not configure a special engine provider, then you could not keep using the estimator. The logic needed an exception for the default engine.

This PR targets the "plugin engine" feature branch.

@fcharras @jjerphan @ogrisel

Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for adding this test case, @betatim. :)

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@jjerphan
Copy link
Member

jjerphan commented Feb 6, 2023

I think we can give a prefix to this PR. I think MAINT is adapted here (it is general maintenance).

What do you think, Tim? 🙂

@ogrisel
Copy link
Member

ogrisel commented Feb 6, 2023

I think MAINT is more for non functional code changes (release, style reformatting or another code reorg that does not change the user-facing behavior of the code base).

Here is more like a FIX to not raise an exception in a context where it should not have been raised in the first place.

There is also ENH for code behavior improvement for functional changes that improve the behavior for something that was not really considered as a bug but more like a limitation of the previous version of the code.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM, this is a net improvement. There are still other test failures but they seem unrelated to the focus of this PR, so let's fix them in follow-up sub PRs to the feature branch.

@ogrisel ogrisel merged commit d19352e into scikit-learn:feature/engine-api Feb 6, 2023
@betatim betatim deleted the fix-default-engine-selector branch February 6, 2023 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants