-
Notifications
You must be signed in to change notification settings - Fork 19.6k
Fix missing and fragile scikit-learn imports in Keras sklearn wrappers #21387
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #21387 +/- ##
=======================================
Coverage 82.67% 82.67%
=======================================
Files 565 565
Lines 55064 55068 +4
Branches 8569 8569
=======================================
+ Hits 45525 45529 +4
Misses 7441 7441
Partials 2098 2098
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Thanks for the PR.
where the SKLearnClassifier and SKLearnRegressor wrappers raise an AttributeError when used without other scikit-learn utilities (e.g. make_classification).
Why does this happen exactly?
It's generally much cleaner to only try to import sklearn
.
Thanks for the review! The issue is that Switching to directly importing |
This is an issue with the design of sklearn; if the package was well-designed then you could access any member of the API from To route around it I would suggest leaving the top-of-file import structure unchanged, and then doing e.g. |
Yes I agree, and this is only the case for After fixing that import, I adjusted the other sklearn imports for consistency by using the same But, if the preference is to use inline imports, I propose to revert all non-essential import changes and leave only the inline import for (Edit): I also found that |
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, thanks for the update!
This PR addresses a bug where the
SKLearnClassifier
andSKLearnRegressor
wrappers raise anAttributeError
when used without otherscikit-learn
utilities (e.g.make_classification
).Fixes
sklearn.utils.multiclass.type_of_target
instead of relying on indirect access viasklearn.utils
make_pipeline, OneHotEncoder, MetadataRequest, check_is_fitted
)sklearn.utils._array_api
by usingnp.squeeze
directly, which is consistent with the docstring expectationsRelated Issue
Fixes #21386