-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
Common check for sample weight invariance with removed samples #16507
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
Changes from all commits
85dc97e
95a2d5b
bc285fa
9533cad
24acb53
662a91f
54db907
2f541ca
ec8542b
0028872
fe9910e
6126e33
a9ebc8f
dcc52f0
137f0d6
ed7abce
5beadf7
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 |
---|---|---|
|
@@ -237,6 +237,14 @@ def fit(self, X, y, sample_weight=None): | |
|
||
return self | ||
|
||
def _more_tags(self): | ||
return { | ||
'_xfail_test': { | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} | ||
|
||
|
||
class LinearSVR(RegressorMixin, LinearModel): | ||
"""Linear Support Vector Regression. | ||
|
@@ -408,6 +416,14 @@ def fit(self, X, y, sample_weight=None): | |
|
||
return self | ||
|
||
def _more_tags(self): | ||
return { | ||
'_xfail_test': { | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} | ||
|
||
|
||
class SVC(BaseSVC): | ||
"""C-Support Vector Classification. | ||
|
@@ -625,6 +641,14 @@ def __init__(self, C=1.0, kernel='rbf', degree=3, gamma='scale', | |
break_ties=break_ties, | ||
random_state=random_state) | ||
|
||
def _more_tags(self): | ||
return { | ||
'_xfail_test': { | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} | ||
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. Of course the amount of repetitions could be reduced by defining this tag in |
||
|
||
|
||
class NuSVC(BaseSVC): | ||
"""Nu-Support Vector Classification. | ||
|
@@ -833,7 +857,9 @@ def _more_tags(self): | |
'_xfail_test': { | ||
'check_methods_subset_invariance': | ||
'fails for the decision_function method', | ||
'check_class_weight_classifiers': 'class_weight is ignored.' | ||
'check_class_weight_classifiers': 'class_weight is ignored.', | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} | ||
|
||
|
@@ -986,6 +1012,14 @@ def probA_(self): | |
def probB_(self): | ||
return self._probB | ||
|
||
def _more_tags(self): | ||
return { | ||
'_xfail_test': { | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} | ||
|
||
|
||
class NuSVR(RegressorMixin, BaseLibSVM): | ||
"""Nu Support Vector Regression. | ||
|
@@ -1111,6 +1145,14 @@ def __init__(self, nu=0.5, C=1.0, kernel='rbf', degree=3, | |
probability=False, cache_size=cache_size, class_weight=None, | ||
verbose=verbose, max_iter=max_iter, random_state=None) | ||
|
||
def _more_tags(self): | ||
return { | ||
'_xfail_test': { | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} | ||
|
||
|
||
class OneClassSVM(OutlierMixin, BaseLibSVM): | ||
"""Unsupervised Outlier Detection. | ||
|
@@ -1319,3 +1361,11 @@ def probA_(self): | |
@property | ||
def probB_(self): | ||
return self._probB | ||
|
||
def _more_tags(self): | ||
return { | ||
'_xfail_test': { | ||
'check_sample_weights_invariance(kind=zeros)': | ||
'zero sample_weight is not equivalent to removing samples', | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,8 @@ def _yield_checks(name, estimator): | |
yield check_sample_weights_not_an_array | ||
yield check_sample_weights_list | ||
yield check_sample_weights_shape | ||
yield check_sample_weights_invariance | ||
yield partial(check_sample_weights_invariance, kind='ones') | ||
yield partial(check_sample_weights_invariance, kind='zeros') | ||
yield check_estimators_fit_returns_self | ||
yield partial(check_estimators_fit_returns_self, readonly_memmap=True) | ||
|
||
|
@@ -443,6 +444,7 @@ def check_estimator(Estimator, generate_only=False): | |
if isinstance(Estimator, type): | ||
# got a class | ||
checks_generator = _generate_class_checks(Estimator) | ||
estimator = _construct_instance(Estimator) | ||
else: | ||
# got an instance | ||
estimator = Estimator | ||
|
@@ -452,12 +454,19 @@ def check_estimator(Estimator, generate_only=False): | |
if generate_only: | ||
return checks_generator | ||
|
||
xfail_checks = _safe_tags(estimator, '_xfail_test') | ||
|
||
for estimator, check in checks_generator: | ||
check_name = _set_check_estimator_ids(check) | ||
if xfail_checks and check_name in xfail_checks: | ||
rth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
# skip tests marked as a known failure and raise a warning | ||
msg = xfail_checks[check_name] | ||
warnings.warn(f'Skipping {check_name}: {msg}', SkipTestWarning) | ||
continue | ||
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 a change following #16502 analogous to what was added in Skipping with a warning tests marked as xfail in
Comment on lines
+462
to
+465
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. Can we apply the xfail decorator as a function instead of manually replicating its behavior? I.e. check = pytest.mark.xfail(check) or something like that? Because we have the try / except logic just below (and we might want to update the comment about pandas)
Member
Author
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. We can't use pytest in this file, since it's supposed to work without it. There is indeed some slight redundancy with _mark_xfail_checks but I think trying to factorize it might be more trouble than what it's worth. It's just 6 extra lines in the end. Updated the comment about pandas. 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'd say the introduction of the tag is a good reason to start requiring
Sure, though I find our whole test suite really hard to maintain in general. 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.
Agreed, I'm just saying that slightly more verbosity and 2 repetitions is easier to maintain than coming up with some function wrappers. The problem is not so much lines of code as complexity. |
||
try: | ||
check(estimator) | ||
except SkipTest as exception: | ||
# the only SkipTest thrown currently results from not | ||
# being able to import pandas. | ||
# raise warning for tests that are are skipped | ||
warnings.warn(str(exception), SkipTestWarning) | ||
|
||
|
||
|
@@ -816,7 +825,7 @@ def check_sample_weights_shape(name, estimator_orig): | |
|
||
|
||
@ignore_warnings(category=FutureWarning) | ||
def check_sample_weights_invariance(name, estimator_orig): | ||
def check_sample_weights_invariance(name, estimator_orig, kind="ones"): | ||
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. Should this check be tested? |
||
# check that the estimators yield same results for | ||
# unit weights and no weights | ||
if (has_fit_parameter(estimator_orig, "sample_weight") and | ||
|
@@ -832,25 +841,45 @@ def check_sample_weights_invariance(name, estimator_orig): | |
X = np.array([[1, 3], [1, 3], [1, 3], [1, 3], | ||
[2, 1], [2, 1], [2, 1], [2, 1], | ||
[3, 3], [3, 3], [3, 3], [3, 3], | ||
[4, 1], [4, 1], [4, 1], [4, 1]], dtype=np.dtype('float')) | ||
[4, 1], [4, 1], [4, 1], [4, 1]], dtype=np.float64) | ||
y = np.array([1, 1, 1, 1, 2, 2, 2, 2, | ||
1, 1, 1, 1, 2, 2, 2, 2], dtype=np.dtype('int')) | ||
1, 1, 1, 1, 2, 2, 2, 2], dtype=np.int) | ||
|
||
if kind == 'ones': | ||
X2 = X | ||
y2 = y | ||
sw2 = np.ones(shape=len(y)) | ||
err_msg = (f"For {name} sample_weight=None is not equivalent to " | ||
f"sample_weight=ones") | ||
elif kind == 'zeros': | ||
# Construct a dataset that is very different to (X, y) if weights | ||
# are disregarded, but identical to (X, y) given weights. | ||
X2 = np.vstack([X, X + 1]) | ||
y2 = np.hstack([y, 3 - y]) | ||
sw2 = np.ones(shape=len(y) * 2) | ||
sw2[len(y):] = 0 | ||
X2, y2, sw2 = shuffle(X2, y2, sw2, random_state=0) | ||
|
||
err_msg = (f"For {name} sample_weight is not equivalent " | ||
f"to removing samples") | ||
else: | ||
raise ValueError | ||
|
||
y = _enforce_estimator_tags_y(estimator1, y) | ||
y2 = _enforce_estimator_tags_y(estimator2, y2) | ||
|
||
estimator1.fit(X, y=y, sample_weight=np.ones(shape=len(y))) | ||
estimator2.fit(X, y=y, sample_weight=None) | ||
estimator1.fit(X, y=y, sample_weight=None) | ||
estimator2.fit(X2, y=y2, sample_weight=sw2) | ||
|
||
for method in ["predict", "transform"]: | ||
for method in ["predict", "predict_proba", | ||
"decision_function", "transform"]: | ||
if hasattr(estimator_orig, method): | ||
X_pred1 = getattr(estimator1, method)(X) | ||
X_pred2 = getattr(estimator2, method)(X) | ||
if sparse.issparse(X_pred1): | ||
X_pred1 = X_pred1.toarray() | ||
X_pred2 = X_pred2.toarray() | ||
assert_allclose(X_pred1, X_pred2, | ||
err_msg="For %s sample_weight=None is not" | ||
" equivalent to sample_weight=ones" | ||
% name) | ||
assert_allclose(X_pred1, X_pred2, err_msg=err_msg) | ||
|
||
|
||
@ignore_warnings(category=(FutureWarning, UserWarning)) | ||
|
Uh oh!
There was an error while loading. Please reload this page.
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.
RidgeClassifierCV now also started to fail with a quite high tolerance required to pass.
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.
Completely unrelated to this PR but since you've been doing this (which is helpful, thanks): do you think it'd be useful for github to support comments from PR author to reviewers, but something distinct from regular review comment?
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.
Maybe, but I can't say the current way it works is an issue for me.