Improve tests to make them run on variously typed data using the `global_dtype` fixture #22881

jjerphan · 2022-03-17T14:24:34Z

Context: the new `global_dtype` fixture and `SKLEARN_RUN_FLOAT32_TESTS` environment variable

Introduction of low-level computational routines for 32bit motivated an extension of tests to run them on 32bit.

In this regards, #22690 introduced a new global_dtype fixture as well has the SKLEARN_RUN_FLOAT32_TESTS env. variable to make it possible to run the test on 32bit data.

Running test on 32bit can be done using SKLEARN_RUN_FLOAT32_TESTS=1.

For instance, this run the first global_dtype-parametrised test:

SKLEARN_RUN_FLOAT32_TESTS=1 pytest sklearn/feature_selection/tests/test_mutual_info.py -k test_compute_mi_cc

This allows running tests on 32bit dataset on some CI job, and currently a single CI job is used to run tests on 32bit.

More details about the fixture in the online dev doc for the SKLEARN_TESTS_GLOBAL_RANDOM_SEED env variable:

https://scikit-learn.org/dev/computing/parallelism.html#environment-variables

Guidelines to convert existing tests

Not all scikit-learn tests must use this fixture. We must parametrise tests that actually assert closeness of results using assert_allclose. For instance, tests that check for the exception messages raised when passing invalid inputs must not be converted.
Tests using np.testing.assert_allclose must now use sklearn.utils._testing.assert_allclose as a drop-in replacement.
Check that the dtype of fitted attributes or return values that depend on the dtype of the input datastructure actually have the expected dtype: typically when all inputs are continuous values in float32, it is often (but not always) the case that scikit-learn should carry all arithmetic operations at that precision level and return output arrays with the same precision level. There can be exceptions, in which case they could be made explicit with an inline comment in the test, possibly with a TODO marker when one thing that the current behavior should change (see the related: Preserving dtype for float32 / float64 in transformers #11000 and Estimator check for dtype preservation for regressors #22682 for instance).
To avoid having to review huge PRs that impact many files at once and can lead to conflicts, let's open PRs that edit at most one test file at a time. For instance use a title such as:

TST use global_dtype in sklearn/_loss/tests/test_glm_distribution.py

Please reference Improve tests to make them run on variously typed data using the global_dtype fixture #22881 (i.e. this issue) in the description of the PR and put the full filename of the test file you edit in the title of the PR.
To convert an existing test with a fixed dtype, the general pattern is to rewrite a function such as:

from numpy.testing import assert_allclose

def test_some_function():
    # ...
    rng = np.random.RandomState(0)
    X = rng.randon.rand(n_samples, n_features)
    y = rng.randon.rand(n_samples).astype(global_dtype)
    model.fit(X, y)
    # ...
    y_pred = model.predict(X)
    assert_allclose(y_pred, y_true)

to:

from sklearn.utils._testing import assert_allclose

def test_some_function(global_dtype):
    # ...
    rng = np.random.RandomState(0)
    X = rng.randon.rand(n_samples, n_features).astype(global_dtype, copy=False)
    y = rng.randon.rand(n_samples).astype(global_dtype, copy=False)
    model.fit(X, y)
    # ...
    assert model.fitted_param_.dtype == global_dtype
    y_pred = model.predict(X)
    assert y_pred.dtype == global_dtype
    assert_allclose(y_pred, y_true)

and then check that the test is passing on 32bit datasets

SKLEARN_RUN_FLOAT32_TESTS=1 pytest sklearn/some_module/test/test_some_module.py -k test_some_function

Failures are to be handle on a case-by-case basis.

List of test modules to upgrade

find sklearn -name "test_*.py"

Note that some of those files might not have any test to update.

The text was updated successfully, but these errors were encountered:

ogrisel · 2022-03-17T14:49:43Z

I have tried to expand the description of this issue, but github fails to take my update into account for some reason.

Here is a copy of the edited issue description:

https://gist.github.com/ogrisel/d6867ca3c8c64e0b3d789f5a2f9ce067

jjerphan · 2022-03-17T15:14:18Z

Thanks, I just pasted your snippet in the description to replace the text.

jjerphan · 2022-03-18T09:51:12Z

Regarding dtype preservation for fitted attributes, wouldn't it be better to introduce global checks for it with tags? I feel that we might spend a lot of time duplicating checks in many tests, potentially inserting a lot of TODO.

To me it seems that this would better be addressed by another meta-issue similar to #11000.

What do you think?

cc @glemaitre

jeremiedbb · 2022-03-18T15:52:21Z

I changed it to hard because it requires some knowledge to figure out which test should use the fixture and which test should not.

jeremiedbb · 2022-03-18T15:55:12Z

I think the list should be updated to remove all estimators that do not preserve the dype yet.
If an estimator doesn't doesn't preserve the dtype, it means that it converts the input when the dtype is not float64 (usually) which means that we end up repeating the same test when we parametrize over the global_dtype.

adam2392 · 2022-03-21T15:40:33Z

@jjerphan just tried this for sklearn/tree/tests/test_tree.py and sklearn/ensemble/tests/test_forest.py and I don't think it works because the TreeBuilder has this conversion inside _check_input, which always forces the X input dtype

        elif X.dtype != DTYPE:  # where DTYPE is float32
            # since we have to copy we will make it fortran for efficiency
            X = np.asfortranarray(X, dtype=DTYPE)

ogrisel · 2022-03-22T12:43:32Z

I changed it to hard because it requires some knowledge to figure out which test should use the fixture and which test should not.

I extended the tests of #22806 using a combination of global_dtype and the global_random_seed fixtures and it revealed numerical problem that would not be visible with the default seed. I already fixed the most obvious but I am not 100% sure if this is good enough or not. So indeed I anticipate those PRs to be hard on average.

jjerphan · 2022-03-23T09:35:56Z

@adam2392: some estimators aren't preserving the provided input dtype.

In that case, it still makes sense to extend the tests adding TODO, as explained in the description of this issue:

Check that the dtype of fitted attributes or return values that depend on the dtype of the input datastructure actually have the expected dtype: typically when all inputs are continuous values in float32, it is often (but not always) the case that scikit-learn should carry all arithmetic operations at that precision level and return output arrays with the same precision level. There can be exceptions, in which case they could be made explicit with an inline comment in the test, possibly with a TODO marker when one thing that the current behavior should change (see the related: #11000 and #22682 for instance).

github-actions bot added the Needs Triage Issue requires triage label Mar 17, 2022

jjerphan added good first issue Easy with clear instructions to resolve help wanted Meta-issue General issue associated to an identified list of tasks labels Mar 18, 2022

jjerphan mentioned this issue Mar 18, 2022

TST use global_dtype in sklearn/cluster/tests/test_affinity_propagation.py #22667

Merged

jjerphan added the float32 Issues related to support for 32bit data label Mar 18, 2022

This was referenced Mar 18, 2022

ENH Add dtype preservation to LocalOutlierFactor #22665

Merged

TST use global_dtype in sklearn/cluster/tests/test_birch.py #22671

Merged

jeremiedbb added Hard Hard level of difficulty and removed good first issue Easy with clear instructions to resolve labels Mar 18, 2022

jjerphan removed the Needs Triage Issue requires triage label Mar 18, 2022

adam2392 mentioned this issue Mar 23, 2022

TST Add TODO for global_dtype in sklearn/tree/tests/test_tree.py #22926

Closed

adam2392 mentioned this issue Mar 24, 2022

TST Add TODO for global_dtype in sklearn/ensemble/tests/test_forest.py #22941

Closed

thomasjpfan mentioned this issue May 4, 2022

TST Speed-up test_predict for KMeans #23274

Merged

cmarmo added the module:test-suite everything related to our tests label Sep 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve tests to make them run on variously typed data using the `global_dtype` fixture #22881

Improve tests to make them run on variously typed data using the `global_dtype` fixture #22881

jjerphan commented Mar 17, 2022 •

edited by jeremiedbb

Loading

ogrisel commented Mar 17, 2022

jjerphan commented Mar 17, 2022

jjerphan commented Mar 18, 2022 •

edited

Loading

jeremiedbb commented Mar 18, 2022

jeremiedbb commented Mar 18, 2022

adam2392 commented Mar 21, 2022

ogrisel commented Mar 22, 2022

jjerphan commented Mar 23, 2022

Improve tests to make them run on variously typed data using the global_dtype fixture #22881

Improve tests to make them run on variously typed data using the global_dtype fixture #22881

Comments

jjerphan commented Mar 17, 2022 • edited by jeremiedbb Loading

Context: the new global_dtype fixture and SKLEARN_RUN_FLOAT32_TESTS environment variable

Guidelines to convert existing tests

List of test modules to upgrade

ogrisel commented Mar 17, 2022

jjerphan commented Mar 17, 2022

jjerphan commented Mar 18, 2022 • edited Loading

jeremiedbb commented Mar 18, 2022

jeremiedbb commented Mar 18, 2022

adam2392 commented Mar 21, 2022

ogrisel commented Mar 22, 2022

jjerphan commented Mar 23, 2022

Improve tests to make them run on variously typed data using the `global_dtype` fixture #22881

Improve tests to make them run on variously typed data using the `global_dtype` fixture #22881

jjerphan commented Mar 17, 2022 •

edited by jeremiedbb

Loading

Context: the new `global_dtype` fixture and `SKLEARN_RUN_FLOAT32_TESTS` environment variable

jjerphan commented Mar 18, 2022 •

edited

Loading