-
-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
[MRG+ ½] Fixing #7155 in stochastic_gradient.py #7159
Changes from all commits
5da25f5
dcbe371
3aa26ac
db59c22
9aed2b5
bc1a700
91b648e
899d0d1
b2c5865
e2fe921
60f2ed1
80ac0e4
d84f5f3
9e3eacb
cb274d4
5594f05
0b3634c
be9ae6f
e1770fa
265fc70
23b325b
2524e3c
7afe3b9
62de9c3
fa6b974
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,16 +14,22 @@ class _IffHasAttrDescriptor(object): | |
"""Implements a conditional property using the descriptor protocol. | ||
|
||
Using this class to create a decorator will raise an ``AttributeError`` | ||
if the ``attribute_name`` is not present on the base object. | ||
if none of the delegates (specified in ``delegate_names``) is an attribute | ||
of the base object or none of the delegates has an attribute | ||
``attribute_name``. | ||
|
||
This allows ducktyping of the decorated method based on ``attribute_name``. | ||
This allows ducktyping of the decorated method based on | ||
``delegate.attribute_name`` where ``delegate`` is the first item in | ||
``delegate_names`` that is an attribute of the base object. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this sentence hard to understand. maybe make it two sentences. And say "Here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about the following: Using this class to create a decorator for a class method This allows ducktyping of the decorated method There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like " for a class method
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's just spit the second sentence as @amueller suggested. |
||
|
||
See https://docs.python.org/3/howto/descriptor.html for an explanation of | ||
descriptors. | ||
""" | ||
def __init__(self, fn, attribute_name): | ||
def __init__(self, fn, delegate_names, attribute_name): | ||
self.fn = fn | ||
self.get_attribute = attrgetter(attribute_name) | ||
self.delegate_names = delegate_names | ||
self.attribute_name = attribute_name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now a tuple. The variable name should reflect this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I.e. should be |
||
|
||
# update the docstring of the descriptor | ||
update_wrapper(self, fn) | ||
|
||
|
@@ -32,7 +38,17 @@ def __get__(self, obj, type=None): | |
if obj is not None: | ||
# delegate only on instances, not the classes. | ||
# this is to allow access to the docstrings. | ||
self.get_attribute(obj) | ||
for delegate_name in self.delegate_names: | ||
try: | ||
delegate = attrgetter(delegate_name)(obj) | ||
except AttributeError: | ||
continue | ||
else: | ||
getattr(delegate, self.attribute_name) | ||
break | ||
else: | ||
attrgetter(self.delegate_names[-1])(obj) | ||
|
||
# lambda, but not partial, allows help() to work with update_wrapper | ||
out = lambda *args, **kwargs: self.fn(obj, *args, **kwargs) | ||
# update the docstring of the returned function | ||
|
@@ -46,27 +62,18 @@ def if_delegate_has_method(delegate): | |
This enables ducktyping by hasattr returning True according to the | ||
sub-estimator. | ||
|
||
>>> from sklearn.utils.metaestimators import if_delegate_has_method | ||
>>> | ||
>>> | ||
>>> class MetaEst(object): | ||
... def __init__(self, sub_est): | ||
... self.sub_est = sub_est | ||
... | ||
... @if_delegate_has_method(delegate='sub_est') | ||
... def predict(self, X): | ||
... return self.sub_est.predict(X) | ||
... | ||
>>> class HasPredict(object): | ||
... def predict(self, X): | ||
... return X.sum(axis=1) | ||
... | ||
>>> class HasNoPredict(object): | ||
... pass | ||
... | ||
>>> hasattr(MetaEst(HasPredict()), 'predict') | ||
True | ||
>>> hasattr(MetaEst(HasNoPredict()), 'predict') | ||
False | ||
Parameters | ||
---------- | ||
delegate : string, list of strings or tuple of strings | ||
Name of the sub-estimator that can be accessed as an attribute of the | ||
base object. If a list or a tuple of names are provided, the first | ||
sub-estimator that is an attribute of the base object will be used. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would leave the doctests as they were or add them as tests to sklearn/utils/tests/test_metaestimators.py. You could also add additional tests in the same file as you did at some point of this PR. Adding tests for if_fitted_delegate_has_method would be good too. |
||
""" | ||
return lambda fn: _IffHasAttrDescriptor(fn, '%s.%s' % (delegate, fn.__name__)) | ||
if isinstance(delegate, list): | ||
delegate = tuple(delegate) | ||
if not isinstance(delegate, tuple): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think list should also be supported. |
||
delegate = (delegate,) | ||
|
||
return lambda fn: _IffHasAttrDescriptor(fn, delegate, | ||
attribute_name=fn.__name__) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,11 @@ | ||
import warnings | ||
import numpy as np | ||
from nose.tools import assert_true, assert_false | ||
from sklearn.utils.metaestimators import if_delegate_has_method | ||
from nose.tools import assert_true | ||
from sklearn.linear_model import SGDClassifier | ||
with warnings.catch_warnings(): | ||
warnings.simplefilter('ignore') | ||
from sklearn.grid_search import GridSearchCV | ||
|
||
|
||
class Prefix(object): | ||
|
@@ -24,3 +30,87 @@ def test_delegated_docstring(): | |
in str(MockMetaEstimator.func.__doc__)) | ||
assert_true("This is a mock delegated function" | ||
in str(MockMetaEstimator().func.__doc__)) | ||
|
||
|
||
def test_stochastic_gradient_loss_param(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test doesn't really belong to test_metaestimators.py maybe somewhere like sklearn/tests/test_grid_search.py would be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for moving it to |
||
# Make sure the predict_proba works when loss is specified | ||
# as one of the parameters in the param_grid. | ||
param_grid = { | ||
'loss': ['log'], | ||
} | ||
X = np.arange(20).reshape(5, -1) | ||
y = [0, 0, 1, 1, 1] | ||
clf = GridSearchCV(estimator=SGDClassifier(loss='hinge'), | ||
param_grid=param_grid) | ||
|
||
# When the estimator is not fitted, `predict_proba` is not available as the | ||
# loss is 'hinge'. | ||
assert_false(hasattr(clf, "predict_proba")) | ||
clf.fit(X, y) | ||
clf.predict_proba(X) | ||
clf.predict_log_proba(X) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to be sure, please assert that when |
||
|
||
# Make sure `predict_proba` is not available when setting loss=['hinge'] | ||
# in param_grid | ||
param_grid = { | ||
'loss': ['hinge'], | ||
} | ||
clf = GridSearchCV(estimator=SGDClassifier(loss='hinge'), | ||
param_grid=param_grid) | ||
assert_false(hasattr(clf, "predict_proba")) | ||
clf.fit(X, y) | ||
assert_false(hasattr(clf, "predict_proba")) | ||
|
||
|
||
class MetaEst(object): | ||
"""A mock meta estimator""" | ||
def __init__(self, sub_est, better_sub_est=None): | ||
self.sub_est = sub_est | ||
self.better_sub_est = better_sub_est | ||
|
||
@if_delegate_has_method(delegate='sub_est') | ||
def predict(self): | ||
pass | ||
|
||
|
||
class MetaEstTestTuple(MetaEst): | ||
"""A mock meta estimator to test passing a tuple of delegates""" | ||
|
||
@if_delegate_has_method(delegate=('sub_est', 'better_sub_est')) | ||
def predict(self): | ||
pass | ||
|
||
|
||
class MetaEstTestList(MetaEst): | ||
"""A mock meta estimator to test passing a list of delegates""" | ||
|
||
@if_delegate_has_method(delegate=['sub_est', 'better_sub_est']) | ||
def predict(self): | ||
pass | ||
|
||
|
||
class HasPredict(object): | ||
"""A mock sub-estimator with predict method""" | ||
|
||
def predict(self): | ||
pass | ||
|
||
|
||
class HasNoPredict(object): | ||
"""A mock sub-estimator with no predict method""" | ||
pass | ||
|
||
|
||
def test_if_delegate_has_method(): | ||
assert_true(hasattr(MetaEst(HasPredict()), 'predict')) | ||
assert_false(hasattr(MetaEst(HasNoPredict()), 'predict')) | ||
assert_false( | ||
hasattr(MetaEstTestTuple(HasNoPredict(), HasNoPredict()), 'predict')) | ||
assert_true( | ||
hasattr(MetaEstTestTuple(HasPredict(), HasNoPredict()), 'predict')) | ||
assert_false( | ||
hasattr(MetaEstTestTuple(HasNoPredict(), HasPredict()), 'predict')) | ||
assert_false( | ||
hasattr(MetaEstTestList(HasNoPredict(), HasPredict()), 'predict')) | ||
assert_true( | ||
hasattr(MetaEstTestList(HasPredict(), HasPredict()), 'predict')) |
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.
"none of the delegates has" -> "the first found delegate does not have"