Skip to content
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.

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

MNT Remove DeprecationWarning for scipy.sparse.linalg.cg tol vs rtol argument #26814

Merged
merged 12 commits into from Aug 18, 2023

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Jul 10, 2023

Fix most of scipy-dev issues seen in #26791

Looking at
https://github.com/scipy/scipy/pull/18488/files#diff-b65f85e6dd5881e4611b34a584512f8df370e520758c30e8d297e70c61dcf48f, it does not seem there is a way to keep the atol="legacy" behaviour. This means that potentially there will be a behaviour change for our users ...

@github-actions
Copy link

github-actions bot commented Jul 10, 2023

❌ Linting issues

There was an issue running the linter job. Please update with upstream/main (link) and push the changes. If you already have done that, please send an empty commit with git commit --allow-empty and push the changes to trigger the CI.

Generated for commit: 8d43a31. Link to the linter CI: here

@lesteve
Copy link
Member Author

lesteve commented Jul 10, 2023

OK there is still one issue with np.byte_bounds being deprecated and used in joblib, this was already noted in joblib/joblib#1454, and can be tackled in a separate PR.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Looks like this PR is conflicting with #26813

sklearn/utils/fixes.py Outdated Show resolved Hide resolved
@ogrisel
Copy link
Member

ogrisel commented Jul 11, 2023

I wonder if the new tol criterion has an impact on the ongoing PR that aims to check good convergence tolerance #25948.

@lesteve
Copy link
Member Author

lesteve commented Jul 12, 2023

I took test_ridge.py from #25948 and enabled the tests with fit_intercept=False and ran the tests for sparse_cg, the intercept=False seems still fail:

pytest sklearn/linear_model/tests/test_ridge.py -k sparse_cg -ra
FAILED sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_unpenalized[wide-42-False-sparse_cg] - AssertionError: 
FAILED sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_unpenalized_hstacked_X[wide-42-False-sparse_cg] - AssertionError: 
FAILED sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_unpenalized_vstacked_X[wide-42-False-sparse_cg] - AssertionError: 

Let me know in case you had something else in mind!

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I do not see a better alternative to this PR. LGTM

@lesteve
Copy link
Member Author

lesteve commented Aug 8, 2023

Note the remaining errors all look like some formatting changes for numpy scalars in numpy development version, better to leave them for a separate PR. This is due to numpy/numpy#22449 that implements NEP 51.

For example:

E       AssertionError: Regex pattern did not match.
E        Regex: "The classes, \\['dog'\\], are not in class_weight"
E        Input: "The classes, [np.str_('dog')], are not in class_weight"

Copy link
Member

@glemaitre glemaitre 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.

@glemaitre
Copy link
Member

I think it is worth to acknowledge the changes in the Changed models section.
I agree with @thomasjpfan that we might not have a better way to do handle this case.

@lesteve
Copy link
Member Author

lesteve commented Aug 18, 2023

Merging my own PR with two approvals 😅

@lesteve lesteve merged commit ff49a91 into scikit-learn:main Aug 18, 2023
24 of 28 checks passed
@lesteve lesteve deleted the sparse-cg-rtol branch August 18, 2023 11:53
@lesteve
Copy link
Member Author

lesteve commented Aug 18, 2023

It looks like the remaining failures may be fixed by #27042

TamaraAtanasoska pushed a commit to TamaraAtanasoska/scikit-learn that referenced this pull request Aug 21, 2023
akaashpatelmns pushed a commit to akaashp2000/scikit-learn that referenced this pull request Aug 25, 2023
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
elindgren pushed a commit to elindgren/scikit-learn that referenced this pull request Oct 5, 2023
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants