Common check for sample weight invariance with removed samples by rth · Pull Request #16507 · scikit-learn/scikit-learn

rth · 2020-02-20T23:14:09Z

Continues and closes #15015

This merges the common check which ensure that setting sample_weight to 0 is equivalent to removing samples. Estimators that currently fail it are listed in #16298 and are marked as a known failure.

We use the _xfail_test estimator tag to mark estimators that xfail this test.

Also related to #11316, #15657

…test

…l_test

rth · 2020-02-20T23:16:18Z

The current output of this test is,

pytest sklearn/tests/test_common.py -k "check_sample_weights_invariance and zeros"       
========================================================= test session starts =========================================================
platform linux -- Python 3.8.0, pytest-5.2.1, py-1.8.0, pluggy-0.13.0                                                                  
rootdir: /home/rth/src/scikit-learn, inifile: setup.cfg                                                                                
plugins: forked-1.1.3, xdist-1.31.0
collected 6243 items / 6078 deselected / 165 selected                                                                                 

sklearn/tests/test_common.py ...............x..................................x...x.....x...............xx...x........x....... [ 59%]
......xx...x................x...............xxxx...................                                                             [100%]

======================================================= short test summary info =======================================================
XFAIL sklearn/tests/test_common.py::test_estimators[CalibratedClassifierCV(base_estimator=LinearDiscriminantAnalysis())-check_sample_we
ights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[IsolationForest()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[KMeans()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[KernelDensity()-check_sample_weights_invariance(kind=zeros)]
  sample_weight must have positive values
XFAIL sklearn/tests/test_common.py::test_estimators[LinearSVC()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[LinearSVR()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[LogisticRegressionCV()-check_sample_weights_invariance(kind=zeros)]                
  zero sample_weight is not equivalent to removing samples                                                                             
XFAIL sklearn/tests/test_common.py::test_estimators[MiniBatchKMeans()-check_sample_weights_invariance(kind=zeros)]                     
  zero sample_weight is not equivalent to removing samples                                                                             
XFAIL sklearn/tests/test_common.py::test_estimators[NuSVC()-check_sample_weights_invariance(kind=zeros)]                               
  zero sample_weight is not equivalent to removing samples                                                                             
XFAIL sklearn/tests/test_common.py::test_estimators[NuSVR()-check_sample_weights_invariance(kind=zeros)]                               
  zero sample_weight is not equivalent to removing samples                                                                             
XFAIL sklearn/tests/test_common.py::test_estimators[OneClassSVM()-check_sample_weights_invariance(kind=zeros)]                        
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[RANSACRegressor(base_estimator=Ridge())-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[SGDClassifier()-check_sample_weights_invariance(kind=zeros)]                      
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[SGDRegressor()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[SVC()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
XFAIL sklearn/tests/test_common.py::test_estimators[SVR()-check_sample_weights_invariance(kind=zeros)]
  zero sample_weight is not equivalent to removing samples
==================================== 149 passed, 6078 deselected, 16 xfailed, 15 warnings in 2.38s ====================================

rth · 2020-02-20T23:20:01Z

sklearn/svm/_classes.py

+                'check_sample_weights_invariance(kind=zeros)':
+                'zero sample_weight is not equivalent to removing samples',
+            }
+        }


Of course the amount of repetitions could be reduced by defining this tag in BaseSVC but the expectations is that these estimators would be fixed one by one and it's easier to understand what needs fixing this way.

rth · 2020-02-21T00:18:48Z

sklearn/utils/estimator_checks.py

+            # 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


This is a change following #16502 analogous to what was added in test_common.py::test_estimators . Without this test_check_estimator_clones started to fail with this PR, since it runs check_estimator on MiniBatchKMeans which now has one xfail test.

Skipping with a warning tests marked as xfail in check_estimator, as done here, is a solution to this issue.

rth · 2020-03-04T15:28:43Z

To answer the question why SVC fails even though it should have been fixed in #14286, the failures of,

$ pytest sklearn/tests/test_common.py -k "check_sample_weights_invariance and (SVC or SVR)" --runxfail

are,

Not equal to tolerance rtol=1e-07, atol=0
E                   For LinearSVC sample_weight is not equivalent to removing samples
E                   Mismatch: 100%
E                   Max absolute difference: 0.03157996
E                   Max relative difference: 0.03572515

E                   For LinearSVR sample_weight is not equivalent to removing samples
E                   Mismatch: 100%
E                   Max absolute difference: 0.17451726
E                   Max relative difference: 0.16016769

E                   For NuSVC sample_weight is not equivalent to removing samples
E                   Mismatch: 100%
E                   Max absolute difference: 2.06101805e-05
E                   Max relative difference: 2.06106053e-05

E                   For NuSVR sample_weight is not equivalent to removing samples
E                   Mismatch: 100%
E                   Max absolute difference: 0.00042324
E                   Max relative difference: 0.00042301

E                   For SVC sample_weight is not equivalent to removing samples
E                   Mismatch: 100%
E                   Max absolute difference: 0.00053014
E                   Max relative difference: 0.00053037

E                   For SVR sample_weight is not equivalent to removing samples
E                   Mismatch: 100%
E                   Max absolute difference: 0.00065833
E                   Max relative difference: 0.00059826

so LinearSVC and LinearSVR look definitely broken and they were not addressed in #14286

To make SVR, SVC, NuSVR, NuSVC one would need to increase the relative tolerance to 6e-4 which is quite a lot. The test added in that PR checked coefficients with rtol=1e-3. Maybe that's the best we can do for libsvm (any 32bit casting happening internally?), not sure..

agramfort

let's merge this and then we can fix estimators one by one

jnothman

This control over checks is quite satisfying to see in action :)

Is there a reason we are modifying y and not X? Doing so means this check only works for supervised estimators.

jnothman · 2020-03-05T09:51:24Z

sklearn/utils/estimator_checks.py

+            err_msg = (f"For {name} sample_weight=None is not equivalent to "
+                       f"sample_weight=ones")
+        elif kind == 'zeros':
+            X2 = np.vstack([X, X])


Deserves a comment like "Construct a dataset that is very different to (X, y) if weights are disregarded, but identical to (X, y) given weights".

However, this doesn't really work when the estimator is unsupervised. This could pass if weights are disregarded.

rth · 2020-03-05T10:47:32Z

Is there a reason we are modifying y and not X? Doing so means this check only works for supervised estimators.

Good point, thanks! Also added a modification to X.

rth · 2020-03-05T10:48:33Z

sklearn/linear_model/_ridge.py

+                'check_sample_weights_invariance(kind=zeros)':
+                'zero sample_weight is not equivalent to removing samples',
+            }
+        }


RidgeClassifierCV now also started to fail with a quite high tolerance required to pass.

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?

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?

Maybe, but I can't say the current way it works is an issue for me.

NicolasHug

Thanks @rth , a few comments and questions

NicolasHug · 2020-03-05T12:51:29Z

sklearn/linear_model/_ridge.py

+                'check_sample_weights_invariance(kind=zeros)':
+                'zero sample_weight is not equivalent to removing samples',
+            }
+        }


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?

sklearn/utils/estimator_checks.py

NicolasHug · 2020-03-05T13:22:23Z

sklearn/utils/estimator_checks.py

+            # 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


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)

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.

I'd say the introduction of the tag is a good reason to start requiring pytest for using check_estimator now. But agreed that's another story.

It's just 6 extra lines in the end

Sure, though I find our whole test suite really hard to maintain in general.

Sure, though I find our whole test suite really hard to maintain in general.

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.

NicolasHug · 2020-03-05T13:24:57Z

sklearn/utils/estimator_checks.py

+        if kind == 'ones':
+            X2 = X
+            y2 = y
+            sw2 = None


Would it be more natural to set this to ones, and set the SW of estimator1 to None instead?

Indeed, fixed.

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

This reverts commit dcc52f0.

NicolasHug

LGTM, thanks @rth

sklearn/utils/estimator_checks.py

NicolasHug · 2020-03-05T14:44:31Z

sklearn/utils/estimator_checks.py

+            # 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


I'd say the introduction of the tag is a good reason to start requiring pytest for using check_estimator now. But agreed that's another story.

It's just 6 extra lines in the end

Sure, though I find our whole test suite really hard to maintain in general.

NicolasHug · 2020-03-05T14:47:19Z

sklearn/utils/estimator_checks.py


 @ignore_warnings(category=FutureWarning)
-def check_sample_weights_invariance(name, estimator_orig):
+def check_sample_weights_invariance(name, estimator_orig, kind="ones"):


Should this check be tested?

rth · 2020-05-10T13:41:35Z

Should this check be tested?

Well for common tests , there is a risk of false negatives and false positives,

false negatives (i.e. errors by mistake) would break the test suite would be fairly easy to detect since CI would fail..
false positives (i.e. not erroring when they should) doesn't happen here since we had to xfail tests that failed.

Sorry I don't have much availability to write detailed tests for this at the moment. I think it's more useful to have this in master (original PR was 8 month ago) that leave it in its current state; also to avoid blocking #15554

Merging with +2.

rth · 2020-05-10T14:38:52Z

OK, no actually I should have re-run CI since there were outdated changes in check_estimator and the way xfail is handled. Was too optimistic. Fix in #17175

#16507)" This reverts commit 77279d6.

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

scikit-learn#16507)" This reverts commit 77279d6.

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

scikit-learn#16507)" This reverts commit 77279d6.

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

scikit-learn#16507)" This reverts commit 77279d6.

amueller and others added 9 commits September 18, 2019 14:30

add test that zero sample weight means samples are ignored

85dc97e

add predict_proba and decision_function to tests

95a2d5b

Merge remote-tracking branch 'origin/master' into sample_weight_real_…

bc285fa

…test

Merge remote-tracking branch 'upstream/master' into sample_weight_rea…

9533cad

…l_test

Mark failing estimators as xfail

24acb53

Split sample wight invariance tests in two

662a91f

Parametrize check_sample_weights_invariance check with kind parameter

54db907

Shuffle data

2f541ca

lint

ec8542b

rth mentioned this pull request Feb 20, 2020

add common test that zero sample weight means samples are ignored #15015

Closed

14 tasks

rth commented Feb 20, 2020

View reviewed changes

Skip tests marked as xfail in check_estimator

0028872

rth commented Feb 21, 2020

View reviewed changes

rth mentioned this pull request Feb 21, 2020

TST Checks can now skip test based on estimator tag _xfail_test #16510

Merged

rth and others added 2 commits February 21, 2020 16:59

Merge branch 'master' into sample-weight-common-check-xfail

fe9910e

Merge branch 'master' into sample-weight-common-check-xfail

6126e33

agramfort approved these changes Mar 4, 2020

View reviewed changes

rth requested a review from NicolasHug March 5, 2020 08:28

jnothman reviewed Mar 5, 2020

View reviewed changes

Address Joels's comment

a9ebc8f

rth commented Mar 5, 2020

View reviewed changes

NicolasHug reviewed Mar 5, 2020

View reviewed changes

rth and others added 4 commits March 5, 2020 14:56

Update sklearn/utils/estimator_checks.py

dcc52f0

Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

Improve wording of the comment

137f0d6

Address comments

ed7abce

Revert "Update sklearn/utils/estimator_checks.py"

5beadf7

This reverts commit dcc52f0.

NicolasHug approved these changes Mar 5, 2020

View reviewed changes

lorentzenchr mentioned this pull request May 10, 2020

TST Add tests for LinearRegression that sample weights act consistently #15554

Merged

rth changed the title ~~Common check for sample weight invariance with removed samples (continued)~~ Common check for sample weight invariance with removed samples May 10, 2020

rth merged commit 77279d6 into scikit-learn:master May 10, 2020

rth deleted the sample-weight-common-check-xfail branch May 10, 2020 13:41

rth mentioned this pull request May 10, 2020

_xfail_tests -> _xfail_checks & revert changes to check_estimator #17175

Closed

rth added a commit that referenced this pull request May 10, 2020

Revert "Common check for sample weight invariance with removed samples (

818e16a

#16507)" This reverts commit 77279d6.

rth added a commit to rth/scikit-learn that referenced this pull request May 10, 2020

Common check for sample weight invariance with removed samples (sciki…

3deca92

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

This was referenced May 10, 2020

Common check for sample weight invariance with removed samples #17176

Merged

[MRG] MNT Ignore xfail_checks in check_estimator #17219

Closed

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

Common check for sample weight invariance with removed samples (sciki…

1ab9b0c

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020

Revert "Common check for sample weight invariance with removed samples (

fcfc214

scikit-learn#16507)" This reverts commit 77279d6.

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020

Common check for sample weight invariance with removed samples (sciki…

dcb4090

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020

Revert "Common check for sample weight invariance with removed samples (

3d94656

scikit-learn#16507)" This reverts commit 77279d6.

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

Common check for sample weight invariance with removed samples (sciki…

0b7ebc7

…t-learn#16507) Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

Revert "Common check for sample weight invariance with removed samples (

65917b6

scikit-learn#16507)" This reverts commit 77279d6.

lorentzenchr mentioned this pull request Oct 25, 2024

Refactor tests for sample weights #11316

Closed

Uh oh!

Conversation

rth commented Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rth commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth commented Mar 4, 2020

Uh oh!

agramfort left a comment

Choose a reason for hiding this comment

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth commented Mar 5, 2020

Uh oh!

rth Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rth commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rth commented May 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rth commented Feb 20, 2020 •

edited

Loading

rth Mar 5, 2020 •

edited

Loading

rth Mar 5, 2020 •

edited

Loading

rth commented May 10, 2020 •

edited

Loading