[MRG+1] test method subset invariance (Fixes #10420)

[Johayon]

Fixes #10420

• added a test for all estimators to check if any of the methods {predict, predict_proba, decision_function, score_samples, transform} produces a different result if applied on all the data or a subset (in this case all the elements one by one).

• the test currently fails in 4 cases

1. SVC with decision_function (SVC and OneVsOneClassifier decision_function inconsistent on sub-sample #9174)
2. SparsePCA with transform (SparsePCA inconsistent on sub-sample #10431)
3. MiniBatchSparsePCA with transform (SparsePCA inconsistent on sub-sample #10431)
4. BernoulliRBM with score_samples (due to stochasticity; can't easily fix)

[jnothman]

cool!

[jnothman]

for now, make exceptions for these cases. Your test also needs an instructive error message when the assertion fails, and an explicit test. SparsePCA uses a ridge regression where each sample is a target. I don't think this should affect things, but it might. It then uses a global normalisation step which is problematic as in OvO/SVC. Please open a new issue for this. RBM I don't think we can easily fix. It randomly corrupts one feature in each sample. For fixed random seed, the first sample will always have the same feature corrupted... The only alternative I can think of is to make the randoms a function of a cryptographic hash of the data, which is overkill.

[Johayon]

I have created the issue for SparsePCA.
The test seems to fail on Birch on the build for python 3.4 because the absolute tolerance is too low, should I put a higher tolerance 1e-7 or 1e-6 ? (I do not have any issue on my build)

[jnothman]

Check if an increased tolerance works. It's still a bit surprising that that should be necessary.

[jnothman]

use tuples. Like if name, method in [('SVC', 'decision_function'), ...]

tuples are intended for struct-like objects where each field means a different thing. Arrays are usually for homogeneous semantics

[jnothman]

This should probably not be marked WIP anymore :P

[jnothman]

Please add an entry to the change log at doc/whats_new/v0.20.rst under "Changes to estimator checks". Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

[glemaitre]

I would slightly rephrase the comment by replacing one by one by something like "if applied on mini bathes or the whole set"

[glemaitre]

I would personally make a small private function to compute and unpack the data.

def _apply_func(func, X):
    result_full = func(X)
    n_features = X.shape[1]
    result_by_batch = [func(batch.reshape(1, n_features))
                       for batch in X]
    # func can output tuple (e.g. score_samples)
    if type(res_all) == tuple:
        result_full = result_full[0]
        result_by_batch = list(map(lambda x: x[0], result_by_batch))

    return np.ravel(result_full), np.ravel(result_by_batch)

def check_methods_subset_invariance(name, estimator_orig):
    ...
    result_full, result_by_batch = _apply_func(get_attr(estimator, method))
    ...
    assert_allclose(results_full, results_by_batch,
                    atol=1e-7, err_msg=msg)

[glemaitre]

LGTM apart of a change in a comment and some coding style.

[glemaitre]

we can actually move this message before the assert_close in fact.

[Johayon]

@glemaitre you want to put it outside the block for and format it inside the assert_close and SkipTest ?

[glemaitre]

oh my bad I did not see the occurrence in SkipTest. Good as it is

[glemaitre]

It will be easier to read as :

result_full, result_by_batch = _apply_func(
    getattr(estimator, method), X)

[glemaitre]

@Johayon 2 small nitpicks and this is good to be merged once it will be green :)

[glemaitre]

@Johayon Thanks!!!