FIX Fixes tags for passthrough in Pipeline #18820
Conversation
|
Thanks for the PR, let's not merge this until we decide on #18797 (comment) please, I think this is strongly related. In particular the suggested option 2 would fix the original issue in a more general and consistent way |
|
This PR also is a bug fix because of |
There was a problem hiding this comment.
I agree with the remark of @thomasjpfan above. WDYT @NicolasHug shall we merge this independently of the other issues?
| estimator_tags = self.steps[0][1]._get_tags() | ||
| return {'pairwise': estimator_tags.get('pairwise', False)} | ||
| try: | ||
| pairwise = self.steps[0][1]._get_tags().get('pairwise', False) |
There was a problem hiding this comment.
I'd prefer just trying hasattr because otherwise it's not clear what may raise the AttributeError (could be e.g. self.steps).
Also about .get('pairwise', False): I think it's fair to assume that if _get_tags exists, then the key will exist.
There was a problem hiding this comment.
Also about .get('pairwise', False): I think it's fair to assume that if _get_tags exists, then the key will exist.
This opens the discussion of "Which tags should we expect to exist". This becomes a bigger question when BaseEstimator is not expected to be inherited.
There was a problem hiding this comment.
Which tags should we expect to exist
If _get_tags() exists, then all of them should exist I'd say
Also, I'm uncomfortable with having the tag's default defined twice: here, and in sklearn.base. The default might not change anytime soon, but this is still a pattern that we should avoid
There was a problem hiding this comment.
If _get_tags() exists, then all of them should exist I'd say
This feels like something to be tested in check_estimator for "api-only".
|
I understand this is a bugfix for If we agree to use Also I believe that keeping this issue open will be helpful during Monday's discussions. If there's a rush to get things in because of the release, I don't want to block it either. |
Reference Issues/PRs
Fixes #18815
What does this implement/fix? Explain your changes.
This PR makes pipeline a little more forgiving when working with steps that does not have
_get_tags.