-
-
Notifications
You must be signed in to change notification settings - Fork 25.2k
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 SLEP6: raise NotImplementedError for meta-estimators not supporting metadata routing #27389
MNT SLEP6: raise NotImplementedError for meta-estimators not supporting metadata routing #27389
Conversation
CI is green now. |
This is ready for review now @glemaitre |
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.
Only a nitpick. Otherwise LGTM.
sklearn/utils/_metadata_requests.py
Outdated
) | ||
|
||
|
||
class _RoutingNotSupported: |
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.
To be in line with other patter in the library.
class _RoutingNotSupported: | |
class _RoutingNotSupportedMixin: |
def _raise_for_unsupported_routing(obj, method, **kwargs): | ||
"""Raise when metadata routing is enabled and metadata is passed. | ||
|
||
This is used in meta-estimators which have not implemented metadata routing |
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.
I think that we could make it explicit that this is to be used in the desired function when metadata are already passed (e.g. sample_weight
)
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.
added a comment
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.
Only a nitpick. Otherwise LGTM.
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 @adrinjalali
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
…ng metadata routing (scikit-learn#27389)
…ng metadata routing (scikit-learn#27389)
This PR makes all meta-estimators for which we haven't implemented metadata routing yet to raise a
NotImplementedError
if they're used in other meta-estimators (via raising this inget_metadata_routing
).It also makes them raise in
fit
if metadata routing is enabled and any metadata is passed to them.cc @glemaitre @OmarManzoor