Skip to content

TST adapt tol for ridge tests to pass on all random seeds #23017

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
Merged
merged 3 commits into from
Apr 20, 2022
Merged

TST adapt tol for ridge tests to pass on all random seeds #23017

merged 3 commits into from
Apr 20, 2022

Conversation

lorentzenchr
Copy link
Member

Reference Issues/PRs

Fixed #23014.

What does this implement/fix? Explain your changes.

Adapts tolerances to let ridge tests pass on all random seeds

Any other comments?

On my machine, the following passes:

SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" pytest sklearn/linear_model/tests/test_ridge.py

@lorentzenchr lorentzenchr requested a review from jeremiedbb April 1, 2022 13:38
@lorentzenchr
Copy link
Member Author

@jeremiedbb Is there a way to run CI with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" just once and just for test_ridge.py?

@lorentzenchr lorentzenchr added the Quick Review For PRs that are quick to review label Apr 1, 2022
@jeremiedbb
Copy link
Member

@jeremiedbb Is there a way to run CI with SKLEARN_TESTS_GLOBAL_RANDOM_SEED="all" just once and just for test_ridge.py?

Nope. I'm currently working on it :)

thomasjpfan
thomasjpfan reviewed Apr 4, 2022
View reviewed changes
sklearn/linear_model/tests/test_ridge.py
@@ -1663,7 +1663,7 @@ def test_ridge_fit_intercept_sparse(solver, with_sample_weight, global_random_se
sparse_ridge.fit(sp.csr_matrix(X), y, sample_weight=sample_weight)

assert_allclose(dense_ridge.intercept_, sparse_ridge.intercept_)
assert_allclose(dense_ridge.coef_, sparse_ridge.coef_)
assert_allclose(dense_ridge.coef_, sparse_ridge.coef_, rtol=5e-7)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this change required for test_ridge_fit_intercept_sparse to locally pass for you?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to pass for all random seeds.

@jeremiedbb
Copy link
Member

In test_ridge_regression_vstacked_X I noticed that for some seeds, the coef of a feature can be quite small. It seems to be due to how data is generated. atol should be used to compare zeros (or what we consider being a zero). When most coefs are around 1e0, it's hard to ensure a rtol of 1e-7 on a coef around 1e-5. In this situation, I'd set a atol around 1e-8.

@jeremiedbb jeremiedbb mentioned this pull request Apr 19, 2022
Merged
@jeremiedbb jeremiedbb added this to the 1.1 milestone Apr 19, 2022
@jeremiedbb
[all random seeds]
dadca84 
test_ridge_regression_unpenalized_vstacked_X
test_ridge_fit_intercept_sparse
ogrisel
ogrisel approved these changes Apr 20, 2022
View reviewed changes
Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well. Maybe we could merge both this and #23152 to make the tests even more robust to small platform-specifc variations while not making the test too trivial either.

@jeremiedbb
wrong test [all random seeds]
a24a775 
test_ridge_regression_vstacked_X
test_ridge_fit_intercept_sparse
@jeremiedbb
Copy link
Member

Let's merge both

@jeremiedbb jeremiedbb merged commit 566cc46 into scikit-learn:main Apr 20, 2022
This was referenced Apr 20, 2022
Closed
Merged
@lorentzenchr lorentzenchr deleted the fix_ridge_test_precision branch April 20, 2022 17:04
@lorentzenchr lorentzenchr mentioned this pull request Apr 21, 2022
Open
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Apr 29, 2022
@lorentzenchr @jjerphan
TST adapt tol for ridge tests to pass on all random seeds (scikit-lea…
976eccf 
…rn#23017)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomasjpfan thomasjpfan thomasjpfan left review comments

@ogrisel ogrisel ogrisel approved these changes

@TomDLT TomDLT TomDLT approved these changes

@jeremiedbb jeremiedbb Awaiting requested review from jeremiedbb

Assignees
No one assigned
Labels
module:linear_model No Changelog Needed Quick Review For PRs that are quick to review
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

test_ridge_regression_vstacked_X is not stable
5 participants
@lorentzenchr @jeremiedbb @ogrisel @thomasjpfan @TomDLT