ENH make fit_transform and fit_predict composite methods #24585
Conversation
|
Codecov fail can be ignored. |
|
On the principle, I am fine. I am thinking about |
|
The parameters passed to the underlying methods don't depend on whether But that reminds me that we should fix all our composite method to accept kwargs and pass them to the underlying methods. I'll work on that fix. |
|
I agree that this seems like a good thing to do...... |
glemaitre
left a comment
glemaitre
left a comment
There was a problem hiding this comment.
LGTM. There is only the pattern discussed by @thomasjpfan that might be worth looking at.
thomasjpfan
left a comment
thomasjpfan
left a comment
There was a problem hiding this comment.
I'm guessing this can wait for #24027 to be merged first?
| MethodMetadataRequest(router=self, owner=owner, method=method), | ||
| ) | ||
|
|
||
| def __getattr__(self, name): |
There was a problem hiding this comment.
Can there be a docstring to explain what this method is doing? At a glance, it looks like it is dynamically creating the metadata routing for the composite methods.
Also, what do you think of setting COMPOSITE_METHODS during MetadataRequest.__init__? It would increase the size of the object, but we do not need override __getattr__.
There was a problem hiding this comment.
@thomasjpfan that would require setting them as descriptors (which we can do), and to make it dynamic based on the values in COMPOSITE_METHODS it'd need a meta class with __init_sublcass__ or an ABC meta-class. Can surely do, but I'm not sure if it's more understandable/easier than this approach.
4 participants
Fixes #22987
This PR makes
fit_predictandfit_transformcomposite methods whose requests are a combination of the corresponding simple methods.This makes it easier for the user to only care about setting requests for
fitandtransform.This PR is implemented as forcing the user to have the same requests for the two methods if the metadata name is the same, otherwise it raises an error. Making it more flexible would require
fit_transformandfit_predictto do routing, which I think we'd like to avoid.cc @jnothman @thomasjpfan @jnothman @glemaitre