8000 FIX support both instance and class level methods in `_AvailableIfDescriptor.__get__` by harupy · Pull Request #20623 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

FIX support both instance and class level methods in _AvailableIfDescriptor.__get__ #20623

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 28, 2021
Merged

FIX support both instance and class level methods in _AvailableIfDescriptor.__get__ #20623

merged 5 commits into from
Jul 28, 2021

Conversation

harupy
Copy link
Contributor
@harupy harupy commented Jul 28, 2021 •
edited
Loading

Reference Issues/PRs

Fixes #20614

What does this implement/fix? Explain your changes.

Fix this line:

scikit-learn/sklearn/utils/metaestimators.py

Line 114 in 5d88f9a

out = lambda *args, **kwargs: self.fn(obj, *args, **kwargs) # noqa

to:

if obj is None:
    # descriptor is accessed through class (e.g. `Pipeline.score`)
    out = lambda *args, **kwargs: self.fn(*args, **kwargs)
else:
    # descriptor is accessed through instance. (e.g. `Pipline([...]).score`)
    out = lambda *args, **kwargs: self.fn(obj, *args, **kwargs)

Any other comments?

@harupy harupy changed the title Add failing test Fix _AvailableIfDescriptor.__get__ Jul 28, 2021
@harupy harupy marked this pull request as draft July 28, 2021 01:09
@harupy harupy mentioned this pull request Jul 28, 2021
Closed
@WeichenXu123
Copy link

@harupy did you forget to commit one file ?

@harupy
Copy link
Contributor Author
harupy commented Jul 28, 2021 •
edited
Loading

@WeichenXu123 I just wanted to double-check if we can really reproduce the issue first. I'll update _AvailableIfDescriptor.__get__ soon to fix the issue.

@harupy harupy force-pushed the fix-if-available branch from b4df0c8 to d593a8c Compare July 28, 2021 02:38
@ogrisel ogrisel added this to the 1.0 milestone Jul 28, 2021
ogrisel
ogrisel approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member
@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix @harupy. It seems that this PR is ready for review, right? It's still marked as draft.

sklearn/utils/metaestimators.py Outdated
@@ -111,7 +111,10 @@ def __get__(self, obj, owner=None):
)

# lambda, but not partial, allows help() to work with update_wrapper
out = lambda *args, **kwargs: self.fn(obj, *args, **kwargs) # noqa
if obj is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment to state that this makes it possible to use the decorated method as an unbound method, for instance when monkeypatching.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!

@harupy harupy marked this pull request as ready for review July 28, 2021 09:10
@harupy
Copy link
Contributor Author
harupy commented Jul 28, 2021

@ogrisel Thanks for the feedback!

WeichenXu123
WeichenXu123 reviewed Jul 28, 2021
View reviewed changes
sklearn/utils/metaestimators.py Outdated
@@ -111,7 +111,10 @@ def __get__(self, obj, owner=None):
)

# lambda, but not partial, allows help() to work with update_wrapper
out = lambda *args, **kwargs: self.fn(obj, *args, **kwargs) # noqa
if obj is None:
out = lambda *args, **kwargs: self.fn(*args, **kwargs) # noqa
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this branch, we need also do delegation attribute exist checking first (and raise AttributeError if delegation attribute does not exist), and then call self.fn(*args, **kwargs)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g.:

pipeline_instance.predict_proba(X)

it will raise 'predict_proba' attribute not found error when pipeline_instance._final_estimator does not have predict_proba attribute.

Similarly, when we call:

Pipeline.predict_proba(pipeline_instance, X)

it should also do the delegation object attribute exist checking before really call predict_proba on delegated object.

@harupy
Address comments
66a0060 
Signed-off-by: harupy <hkawamura0130@gmail.com>
harupy
harupy commented Jul 28, 2021
View reviewed changes
@glemaitre glemaitre self-requested a review July 28, 2021 15:07
glemaitre
glemaitre approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member
@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@glemaitre glemaitre changed the title Fix _AvailableIfDescriptor.__get__ FIX support both instance and class level methods in _AvailableIfDescriptor.__get__ Jul 28, 2021
@glemaitre glemaitre merged commit f2f8b52 into scikit-learn:main Jul 28, 2021
@glemaitre
Copy link
Member

Thanks @harupy for finding and solving this issue.

@harupy harupy deleted the fix-if-available branch July 28, 2021 16:19
TomDLT pushed a commit to TomDLT/scikit-learn that referenced this pull request Jul 29, 2021
@harupy @TomDLT
FIX support both instance and class level methods in `_AvailableIfDes…
17195d6 
…criptor.__get__` (scikit-learn#20623)
@harupy harupy mentioned this pull request Aug 2, 2021
Merged
samronsin pushed a commit to samronsin/scikit-learn that referenced this pull request Nov 30, 2021
@harupy @samronsin
FIX support both instance and class level methods in `_AvailableIfDes…
7edbcfb 
…criptor.__get__` (scikit-learn#20623)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnothman jnothman jnothman approved these changes

@WeichenXu123 WeichenXu123 WeichenXu123 left review comments

@ogrisel ogrisel ogrisel approved these changes

@glemaitre glemaitre glemaitre approved these changes

Assignees
No one assigned
Labels
Blocker Bug module:utils No Changelog Needed
Projects
None yet
Milestone
1.0
Development

Successfully merging this pull request may close these issues.

Bug when using a function decorated by if_delegate_has_method
5 participants
@harupy @WeichenXu123 @glemaitre @jnothman @ogrisel
0