8000 TST Add option to run tests on 32bit data · Issue #22680 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

TST Add option to run tests on 32bit data #22680

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.

Sign up for GitHub

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

Jump to bottom
Closed
jjerphan opened this issue Mar 3, 2022 · 6 comments
Closed

TST Add option to run tests on 32bit data #22680

jjerphan opened this issue Mar 3, 2022 · 6 comments
Labels
Build / CI float32 Issues related to support for 32bit data module:test-suite everything related to our tests workflow Development workflow changes

Comments

@jjerphan
Copy link
Member
jjerphan commented Mar 3, 2022

Context

Currently most implementations are tested against 64bit datasets only. The re-factoring of some internals for computations on 32bit datasets brought the need to test user-facing interfaces on 32bit datasets (see #22590 (comment)). A few group of PRs (such as #22663) introduced a simple parametrisation on dtype:

# ... 
DTYPES = (np.float32, np.float64)

# ...

@pytest.mark.parametrize("dtype", DTYPES)
def test(dtype):
    # ...

Problem

Yet, parametrising tests on dtype might just make running the test suite (much) longer. Hence, we could add an option to be able to run such tests for 32bit data optionally. In this case, we also need to have a way to specify this option so that it can be triggered on the CI.

Proposed solution

We can keep the tests parametrisation on dtype as proposed in PRs (such as #22663) but have the value of DTYPES be specified globally (e.g. in sklearn/utils/_testing.py) and adaptively on the value environement variable. Naively:

# sklearn/utils/_testing.py

# ...

DTYPES = [np.float64]

if os.environ.get("SKLEARN_RUN_32BIT_TESTS"):
    DTYPES.append(np.float32)

As for the CI, we could have a new [test 32bit] commit message marker which would define this environement variable.
@github-actions github-actions bot added the Needs Triage Issue requires triage label Mar 3, 2022
@jjerphan jjerphan mentioned this issue Mar 3, 2022
Merged
@jjerphan jjerphan added module:test-suite everything related to our tests Build / CI workflow Development workflow changes labels Mar 3, 2022
@jeremiedbb
Copy link
Member

I'm not sure there's a need to run all or almost all tests against float32.
Obviously estimators that convert data to float64 (those for which self._validate_data has dtype=np.float64) should not be tested.
For estimators that are supposed to support float32 without conversion, we need 3 tests:

  • for transformers we test that the transformed array has the same dtype as the input. This already is a common test. For regressors we can check the prediction as discussed in Estimator check for dtype preservation for regressors #22682.
  • A test that check that some specific attributes have the dtype as the data (i.e. cluster_centers_ for KMeans). Which attribute to check is estimator specific and should be decided on a case by case basis.
  • A test that check that the results, i.e the transformed data or some attributes, are the same between float32 and float64. For that we need a bit of tolerance because machine precision of float32 is 1.2e-7 which is bigger than the default rtol of assert_allclose. Besides, rounding errors can accumulate meaning that we should use a rtol around 1e-5 or even 1e-4.

I think having these tests should be enough, in the sense that if we check that the result is the same between float32 and float64 then by transitivity, testing some property of an estimator on float64 is guaranteed to apply to float32 as well.

@jjerphan jjerphan mentioned this issue Mar 4, 2022
Merged
@thomasjpfan thomasjpfan removed the Needs Triage Issue requires triage label Mar 9, 2022
@jjerphan jjerphan added the float32 Issues related to support for 32bit data label Mar 18, 2022
@thomasjpfan
Copy link
Member

Is the plan to convert all tests that parameterize over float32 and float64 to use the fixture?

Lets say that a contributor is adjusting an algorithm to fix a bug that would fail a test for float32, we would not know until the PR is merged and the nightly build runs (assuming #23110 is merged). With a commit trigger like #23026 (comment), there will be more responsibility on the reviewer to remember to run tests for float32 to catch float32 specific bugs before merging.

@jeremiedbb
Copy link
Member

we would not know until the PR is merged and the nightly build runs

The fixture is already enabled on 1 CI job so we should know in the PR (except in rare situations where there's a numerical instability only visible on another platform).

Is the plan to convert all tests that parameterize over float32 and float64 to use the fixture?

At first I didn't think so, but then I thought why not. Transformers that have received the preserves_dtype flag recently have been granted new tests with the fixture. I don't see a reason to treat the other transformers differently. Maybe we shouldn't do that for tests added for a bug fix though (maybe it's already the case :/ ).

Ideally I would always run tests for both precisions but parametrizing more and more tests over float64/float32 makes the CI take more and more time, and it's already too long (without mentioning energy consumption). I think having the fixture on 1 job for the PR and all jobs for the nightly builds is a good compromise. Wdyt ?

@thomasjpfan
Copy link
Member
thomasjpfan commented Apr 12, 2022 •
edited
Loading

I think having the fixture on 1 job for the PR and all jobs for the nightly builds is a good compromise. Wdyt ?

It's a trade off between developer time and CI time. If there is a float32 issue in a PR that is not caught by the one PR job, then it requires more developer time to fix it after it is caught on nightly builds.

Let's enable the "1 job for the PR and all jobs for nightly" plan now. In the long term, I think it depends on how often does the above situation happens. If it's rare, then I'm okay with the compromise.

@jjerphan
Copy link
Member Author

Addressed by #22690.

1 similar comment
@jjerphan
Copy link
Member Author

Addressed by #22690.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Build / CI float32 Issues related to support for 32bit data module:test-suite everything related to our tests workflow Development workflow changes
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@thomasjpfan @jjerphan @jeremiedbb
0