MAINT move _estimator_has function to utils #29319
Conversation
|
@glemaitre Can you please review this? |
There was a problem hiding this comment.
Hey @nithish08,
thanks for your PR. I went through it and left you some comments. I'm not a maintainer though.
Co-authored-by: Stefanie Senger <91849487+StefanieSenger@users.noreply.github.com>
Co-authored-by: Stefanie Senger <91849487+StefanieSenger@users.noreply.github.com>
|
Thanks, @StefanieSenger, for the review. I have addressed the comments. Please re-review when you get a chance. |
There was a problem hiding this comment.
Thank you for your update and further work, @nithish08.
The _estimator_has function and the tests for it look pretty good.
I have left you some comments regarding documentation and I suggest using the pytest.mark.parametrize decorator so the tests are more consistent and easier to read.
Co-authored-by: Stefanie Senger <91849487+StefanieSenger@users.noreply.github.com>
Co-authored-by: Stefanie Senger <91849487+StefanieSenger@users.noreply.github.com>
…nto nb/est_refactor
|
Thanks, @StefanieSenger; I have addressed the comments. |
There was a problem hiding this comment.
Thanks for your work @nithish08, it looks fantastic! 🥇
Pinging @glemaitre to also have a look at it.
Co-authored-by: Stefanie Senger <91849487+StefanieSenger@users.noreply.github.com>
|
Thanks. I'll put in my list for review. |
sklearn/utils/validation.py
Outdated
| # In meta estimators with multiple sub estimators, | ||
| # only the attribute of the first sub estimator is checked, | ||
| # assuming uniformity across all sub estimators. | ||
| if delegate == "estimators_": |
There was a problem hiding this comment.
I don't like this statement here. Basically, we could do the same for estimators as well. But then, we might have third party that have something different and then they cannot use this tool. For instance, in imbalanced-learn, we would use this pattern with samplers_ and samplers for instance.
So I would be a bit more lenient here and only check if the attribute can be iterated:
if hasattr(self, delegate):
delagator = getattr(self, delegate)
if isinstance(delegator, Sequence):
return getattr(delegator[0], attr)
else:
return getattr(delegator, attr)where Sequence can be imported from collections.abc
There was a problem hiding this comment.
I don't think we should care much about third parties since this is very private, but I like your diff anyway.
|
ping @adrinjalali to take over this PR. |
sklearn/utils/validation.py
Outdated
| # In meta estimators with multiple sub estimators, | ||
| # only the attribute of the first sub estimator is checked, | ||
| # assuming uniformity across all sub estimators. | ||
| if delegate == "estimators_": |
There was a problem hiding this comment.
I don't think we should care much about third parties since this is very private, but I like your diff anyway.
Fixes Issue-29046
Moved _estimator_has function to utils.
Side-comment:
_estimator_hasfunction insklearn/model_selection/_search.pyrenamed to_search_estimator_hasto avoid confusion.