[MRG] FIX reimplement StratifiedKFold to avoid variation in test size #14704

jnothman · 2019-08-21T08:52:44Z

TODO:

test_split.py passes here and fails in master
rest of test suite passes
add test for invariance to class label
determine if this breaks too much and needs a slower change (how can I describe which cases are unchanged)
update docs as necessary
update what's new

… size

jnothman · 2019-08-21T10:58:32Z

This is the failure I haven't fixed:

def test_calibration():
        """Test calibration objects with isotonic and sigmoid"""
        n_samples = 100
        X, y = make_classification(n_samples=2 * n_samples, n_features=6,
                                   random_state=42)
        sample_weight = np.random.RandomState(seed=42).uniform(size=y.size)
    
        X -= X.min()  # MultinomialNB only allows positive X
    
        # split train and test
        X_train, y_train, sw_train = \
            X[:n_samples], y[:n_samples], sample_weight[:n_samples]
        X_test, y_test = X[n_samples:], y[n_samples:]
    
        # Naive-Bayes
        clf = MultinomialNB().fit(X_train, y_train, sample_weight=sw_train)
        prob_pos_clf = clf.predict_proba(X_test)[:, 1]
    
        pc_clf = CalibratedClassifierCV(clf, cv=y.size + 1)
        assert_raises(ValueError, pc_clf.fit, X, y)
    
        # Naive Bayes with calibration
        for this_X_train, this_X_test in [(X_train, X_test),
                                          (sparse.csr_matrix(X_train),
                                           sparse.csr_matrix(X_test))]:
            for method in ['isotonic', 'sigmoid']:
                pc_clf = CalibratedClassifierCV(clf, method=method, cv=2)
                # Note that this fit overwrites the fit on the entire training
                # set
                pc_clf.fit(this_X_train, y_train, sample_weight=sw_train)
                prob_pos_pc_clf = pc_clf.predict_proba(this_X_test)[:, 1]
    
                # Check that brier score has improved after calibration
                assert (brier_score_loss(y_test, prob_pos_clf) >
                               brier_score_loss(y_test, prob_pos_pc_clf))
    
                # Check invariance against relabeling [0, 1] -> [1, 2]
                pc_clf.fit(this_X_train, y_train + 1, sample_weight=sw_train)
                prob_pos_pc_clf_relabeled = pc_clf.predict_proba(this_X_test)[:, 1]
                assert_array_almost_equal(prob_pos_pc_clf,
                                          prob_pos_pc_clf_relabeled)
    
                # Check invariance against relabeling [0, 1] -> [-1, 1]
                pc_clf.fit(this_X_train, 2 * y_train - 1, sample_weight=sw_train)
                prob_pos_pc_clf_relabeled = pc_clf.predict_proba(this_X_test)[:, 1]
                assert_array_almost_equal(prob_pos_pc_clf,
                                          prob_pos_pc_clf_relabeled)
    
                # Check invariance against relabeling [0, 1] -> [1, 0]
                pc_clf.fit(this_X_train, (y_train + 1) % 2,
                           sample_weight=sw_train)
                prob_pos_pc_clf_relabeled = \
                    pc_clf.predict_proba(this_X_test)[:, 1]
                if method == "sigmoid":
                    assert_array_almost_equal(prob_pos_pc_clf,
>                                             1 - prob_pos_pc_clf_relabeled)
E                   AssertionError: 
E                   Arrays are not almost equal to 6 decimals
E                   
E                   Mismatch: 100%
E                   Max absolute difference: 0.058605
E                   Max relative difference: 0.17545084
E                    x: array([0.639171, 0.284055, 0.583672, 0.699171, 0.473917, 0.334347,
E                          0.252862, 0.320488, 0.77633 , 0.41827 , 0.430533, 0.707298,
E                          0.259467, 0.829195, 0.070182, 0.793064, 0.276862, 0.56796 ,...
E                    y: array([0.647982, 0.328991, 0.565925, 0.713699, 0.492425, 0.326041,
E                          0.248765, 0.312138, 0.767384, 0.44601 , 0.407218, 0.719722,
E                          0.314678, 0.825227, 0.076316, 0.799289, 0.307708, 0.595152,...

X          = array([[1.55552559, 2.69252392, 3.45217209, 3.20990507, 3.54706644,
        4.39080293],
       [1.46335164, 2.5219545...4072529,
        3.85817712],
       [1.93819423, 2.94549552, 3.979749  , 4.12309001, 3.11853784,
        4.08188049]])
X_test     = array([[3.7431524 , 3.26830574

It's not obvious to me what assertions there should be dependent on CV splitter... Is the test just brittle?

jnothman · 2019-08-21T11:28:14Z

Ah... I see. The incumbent StratifiedKFold is invariant to class label. The new one is not.

Invariance to class labels seems a pretty useful property. And it seems something worth testing more directly in test_split.py if we want to keep it.

One not very satisfying solution would be making the "encoding" dependent not on the lexical ordering of the labels, but on the order in which they appear in the data. That is, rather than

_, y_encoded = np.unique(y, return_inverse=True)

use

_, y_idx, y_inv = np.unique(y, return_index=True, return_inverse=True)
_, class_perm = np.unique(y_idx, return_inverse=True)
y_encoded = class_perm[y_inv]

WDYT?

amueller · 2019-08-21T18:30:10Z

sounds good but calling unique twice to achieve that seems a bit odd?

amueller · 2019-08-21T18:39:26Z

sklearn/model_selection/_split.py

+
+        test_folds = np.empty(len(y), dtype='i')
+        for i in range(n_classes):
+            folds_for_class = np.arange(self.n_splits).repeat(allocation[:, i])


can you maybe add a comment to this? I got it now but it took me a minute ;)

amueller · 2019-08-21T18:44:01Z

some related tests are failing, probably because they need to be updated? It's a bit weird that the tests rely on the implementation details.

Is there a test that if KFold would be stratified then the results are identical (at least without shuffling)?
If we have [1, 2, 3, 1, 2, 3, 1, 2, 3] and do three-fold then KFold and StratifiedKfold should agree, right?

amueller · 2019-08-21T20:49:32Z

otherwise this looks good :)

jnothman · 2019-08-21T20:57:55Z

Is there a test that if KFold would be stratified then the results are identical (at least without shuffling)?

There is a more heuristic test of the data order dependency: test_kfold_can_detect_dependent_samples_on_digits. But no explicit test of KFold-StratifiedKFold equivalent that I've noticed yet...

jnothman · 2019-08-21T21:11:11Z

Yes, the code is pretty hard to follow! I tried to make it easier by not using my count based solution above!!

jnothman · 2019-08-21T23:13:34Z

Ping @wdevazelhes due to #10155. What do you think of the new implementation?

jnothman · 2019-08-21T23:52:53Z

Happy with those comments, @amueller?

wdevazelhes · 2019-08-22T14:12:20Z

Ping @wdevazelhes due to #10155. What do you think of the new implementation?

I had a look and I think it's really nice !

qinhanmin2014

I think we can change the implementation directly because the difference is often small

sklearn/model_selection/_split.py

qinhanmin2014 · 2019-08-23T08:42:50Z

sklearn/model_selection/tests/test_split.py

@@ -368,24 +369,56 @@ def test_stratified_kfold_no_shuffle():
        list(StratifiedKFold(2).split(X, y1)),
        list(StratifiedKFold(2).split(X, y2)))

+    # Check equivalence to KFold


why this test? this seems like a coincidence?

@amueller requested it. Not a coincidence. A matter of ensuring dependencies are maintained

qinhanmin2014 · 2019-08-23T08:42:50Z

sklearn/model_selection/tests/test_search.py

@@ -210,7 +210,7 @@ def check_hyperparameter_searcher_with_fit_params(klass, **klass_kwargs):
                         "Expected fit parameter(s) ['eggs'] not seen.",
                         searcher.fit, X, y, spam=np.ones(10))
    assert_raise_message(AssertionError,
-                         "Fit parameter spam has length 1; expected 4.",
+                         "Fit parameter spam has length 1; expected",


what's happening here?

The fit_params had been indexed according to the cv

qinhanmin2014 · 2019-08-23T08:42:50Z

sklearn/model_selection/tests/test_split.py


-def test_stratified_kfold_ratios():
+@pytest.mark.parametrize('shuffle', [False, True])
+@pytest.mark.parametrize('k', [4, 5, 6, 7, 8, 9, 10])


reduce number of k maybe?

I figure it's a pretty quick test, and I found some implementation bugs appeared only on some K...

qinhanmin2014 · 2019-08-23T08:42:50Z

sklearn/model_selection/_split.py

+
+    * Generate test sets such that all contain the same distribution of
+      classes, or as close as possible.
+    * Be invariant to class label: relabelling ``y = ["Happy", "Sad"]`` to


maybe more straightforward to use y=[0, 1]?

I think the translation is less clear to the reader if I use [0,1]

amueller · 2019-08-24T16:19:35Z

oh yeah!

jnothman · 2019-08-24T23:51:32Z

It is exciting to have this out of the way! I'm a little worried it will annoy people that results have changed....

jnothman added 2 commits August 21, 2019 18:47

FIX replace StratifiedKFold implementation to avoid variation in test…

e67b2cf

… size

Fix tests dependent on former behaviour

022970d

jnothman added 2 commits August 21, 2019 21:31

make splits invariant to class labels

8eda644

Test that stratified k fold is invariant to class labels

9f2e999

amueller reviewed Aug 21, 2019

View changes

jnothman added 4 commits August 22, 2019 08:51

Make tests a little more tolerant

0eb6d0c

Fix test broken due to subtly different splits

863d2bb

Add comments

ec2b6ef

notes

cf27892

jnothman added 3 commits August 22, 2019 09:16

TST StratifiedKFold-KFold equivalence

9b7c629

Add note on preserving order dependencies

68b2db8

What's new

0cc5ff7

jnothman changed the title ~~[WIP] FIX reimplement StratifiedKFold to avoid variation in test size~~ [MRG] FIX reimplement StratifiedKFold to avoid variation in test size Aug 21, 2019

Merge branch 'master' into newstratified

7f85b7e

amueller approved these changes Aug 22, 2019

View changes

qinhanmin2014 approved these changes Aug 23, 2019

View changes

Don't need a stable sort

24dedf0

qinhanmin2014 approved these changes Aug 24, 2019

View changes

qinhanmin2014 merged commit 3de368d into scikit-learn:master Aug 24, 2019

amueller mentioned this pull request Sep 3, 2019

[MRG] Binned regression cv #14560

Open

jnothman mentioned this pull request Dec 4, 2019

StratifiedKFold yields slightly different class label distributions (prior to 0.22 different train and test sizes) #15783

Open

[MRG] FIX reimplement StratifiedKFold to avoid variation in test size #14704

[MRG] FIX reimplement StratifiedKFold to avoid variation in test size #14704

jnothman commented Aug 21, 2019 •

edited

jnothman commented Aug 21, 2019 •

edited

jnothman commented Aug 21, 2019 •

edited

amueller commented Aug 21, 2019

amueller Aug 21, 2019

amueller commented Aug 21, 2019

amueller commented Aug 21, 2019

jnothman commented Aug 21, 2019

jnothman commented Aug 21, 2019 via email

jnothman commented Aug 21, 2019

jnothman commented Aug 21, 2019

wdevazelhes commented Aug 22, 2019

qinhanmin2014 left a comment

qinhanmin2014 Aug 23, 2019

jnothman Aug 24, 2019

qinhanmin2014 Aug 23, 2019

jnothman Aug 24, 2019

qinhanmin2014 Aug 23, 2019

jnothman Aug 24, 2019

qinhanmin2014 Aug 23, 2019

jnothman Aug 24, 2019

amueller commented Aug 24, 2019

jnothman commented Aug 24, 2019 via email

[MRG] FIX reimplement StratifiedKFold to avoid variation in test size #14704

[MRG] FIX reimplement StratifiedKFold to avoid variation in test size #14704

Conversation

jnothman commented Aug 21, 2019 • edited

jnothman commented Aug 21, 2019 • edited

jnothman commented Aug 21, 2019 • edited

amueller commented Aug 21, 2019

Choose a reason for hiding this comment

amueller commented Aug 21, 2019

amueller commented Aug 21, 2019

jnothman commented Aug 21, 2019

jnothman commented Aug 21, 2019 via email

jnothman commented Aug 21, 2019

jnothman commented Aug 21, 2019

wdevazelhes commented Aug 22, 2019

qinhanmin2014 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller commented Aug 24, 2019

jnothman commented Aug 24, 2019 via email

jnothman commented Aug 21, 2019 •

edited

jnothman commented Aug 21, 2019 •

edited

jnothman commented Aug 21, 2019 •

edited