Skip to content

[MRG+1] ENH Ridge with solver SAG/SAGA does not cast to float64 #13302

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 19 commits into from
May 4, 2019
Merged

[MRG+1] ENH Ridge with solver SAG/SAGA does not cast to float64 #13302

merged 19 commits into from
May 4, 2019

Conversation

massich
Copy link
Contributor

@massich massich commented Feb 27, 2019 •
edited
Loading

closes #11642, (also closes #11643 by taking over)

build upon #11155

TODO:

@massich massich changed the title Is/ridge regression sag EHN: Ridge with solver SAG/SAGA does not cast to float64 Feb 27, 2019
@massich massich force-pushed the is/ridge_regression_sag branch from 872ebf5 to 5630c25 Compare February 27, 2019 10:33
@massich massich force-pushed the is/ridge_regression_sag branch from 5630c25 to c95f6ba Compare February 27, 2019 10:44
@massich
Copy link
Contributor Author

massich commented Feb 27, 2019

I get a segfall that I don't manage to reproduce

massich
massich commented Feb 28, 2019
View reviewed changes
sklearn/linear_model/tests/test_ridge.py Outdated
assert results[np.float32].dtype == np.float32
assert results[np.float64].dtype == np.float64
assert_allclose(results[np.float32], results[np.float64],
rtol=assert_tolerance)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The types are maintained but the numerical results are not stable. See:

~/code/scikit-learn is/ridge_regression_sag*
(mne) ❯ pytest sklearn/linear_model/tests/test_ridge.py -k dtype_stability -v --tb=no      
Test session starts (platform: linux, Python 3.6.6, pytest 4.0.0, pytest-sugar 0.9.2)
cachedir: .pytest_cache
rootdir: /home/sik/code/scikit-learn, inifile: setup.cfg
plugins: sugar-0.9.2, pudb-0.7.0, mock-1.10.0, faulthandler-1.5.0, cov-2.6.0
collecting ... 
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.1-svd] ✓                       4% ▌         
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.1-cholesky] ✓                  8% ▉         
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.1-lsqr] ✓                     12% █▍        
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.1-sparse_cg] ✓                17% █▋        
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.1-sag] ✓                      21% ██▏       
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.1-saga] ✓                     25% ██▌       
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.01-svd] ✓                     29% ██▉       
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.01-cholesky] ✓                33% ███▍      
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.01-lsqr] ✓                    38% ███▊      
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.01-sparse_cg] ✓               42% ████▎     

 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.01-sag] ⨯                     46% ████▋     
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.01-saga] ✓                    50% █████     
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.001-svd] ✓                    54% █████▌    
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.001-cholesky] ✓               58% █████▉    
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.001-lsqr] ✓                   62% ██████▍   
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.001-sparse_cg] ✓              67% ██████▋   

 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.001-sag] ⨯                    71% ███████▏  

 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[0.001-saga] ⨯                   75% ███████▌  
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[1e-06-svd] ✓                    79% ███████▉  
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[1e-06-cholesky] ✓               83% ████████▍ 

 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[1e-06-lsqr] ⨯                   88% ████████▊ 
 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[1e-06-sparse_cg] ✓              92% █████████▎

 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[1e-06-sag] ⨯                    96% █████████▋

 sklearn/linear_model/tests/test_ridge.py::test_ridge_regression_dtype_stability[1e-06-saga] ⨯                  100% ██████████

Results (0.66s):
      18 passed
       6 failed
         - sklearn/linear_model/tests/test_ridge.py:943 test_ridge_regression_dtype_stability[0.01-sag]
         - sklearn/linear_model/tests/test_ridge.py:943 test_ridge_regression_dtype_stability[0.001-sag]
         - sklearn/linear_model/tests/test_ridge.py:943 test_ridge_regression_dtype_stability[0.001-saga]
         - sklearn/linear_model/tests/test_ridge.py:943 test_ridge_regression_dtype_stability[1e-06-lsqr]
         - sklearn/linear_model/tests/test_ridge.py:943 test_ridge_regression_dtype_stability[1e-06-sag]
         - sklearn/linear_model/tests/test_ridge.py:943 test_ridge_regression_dtype_stability[1e-06-saga]
      49 deselected

Similar issues can be found #13303 and #13309

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what is weird compared to #13309 is that svd is stable

@massich massich marked this pull request as ready for review February 28, 2019 11:16
@GaelVaroquaux
Copy link
Member

There are segfaults in the tests on azure :(

@massich
Copy link
Contributor Author

massich commented Feb 28, 2019

There are segfaults in the tests on azure :(

@GaelVaroquaux :) yes! The thing is that sag only supports csr and we are skipping the check, so it segfaults. If none of the other solvers supports the other things, then just hardcoding csr is enough, otherwise we need to move the validation logic ouside.

@massich
Copy link
Contributor Author

massich commented Feb 28, 2019

I was trying to see which solver prefers which sparse configuration in our doc, and I could not find it:
https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.Ridge.html#sklearn.linear_model.Ridge.fit
https://scikit-learn.org/stable/modules/linear_model.html#ridge-regression
https://scikit-learn.org/stable/modules/generated/sklearn.linear_model.ridge_regression.html#sklearn.linear_model.ridge_regression

on the other side, when we use things like scipy's lsqr I didn't figure out which sparse preference the algorithm has. I guess that clue is the first thing that lsqr does is to call this
https://github.com/scipy/scipy/blob/f2ec91c4908f9d67b5445fbfacce7f47518b35d1/scipy/sparse/linalg/interface.py#L381:5

@massich massich force-pushed the is/ridge_regression_sag branch from 98c4eda to e3df1df Compare February 28, 2019 17:44
@agramfort agramfort changed the title EHN: Ridge with solver SAG/SAGA does not cast to float64 ENH: Ridge with solver SAG/SAGA does not cast to float64 Mar 1, 2019
agramfort
agramfort reviewed Mar 1, 2019
View reviewed changes
Copy link
Member

@agramfort agramfort left a comment

Choose a reason for hiding this comment

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

LGTM.

+1 for MRG if CIs are green

please also update what's new

@massich
Copy link
Contributor Author

massich commented Mar 1, 2019

cross reference #11000

@glemaitre glemaitre mentioned this pull request Mar 1, 2019
Open
28 tasks
@GaelVaroquaux
Copy link
Member

Test failures are legit and easy to fix: it's "RandomState", and not "randomstate"

GaelVaroquaux
GaelVaroquaux reviewed Mar 1, 2019
View reviewed changes
Copy link
Member

@GaelVaroquaux GaelVaroquaux left a comment

Choose a reason for hiding this comment

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

LGTM aside from the randomstate issues in the test.

@jnothman
Copy link
Member

jnothman commented May 1, 2019

This seems to have approvals, but just needs finishing touches. Assuming we don't decide to squeeze it into 0.21 this week, what's new entry should be moved to 0.22.

@glemaitre glemaitre self-assigned this May 2, 2019
@glemaitre glemaitre changed the title ENH: Ridge with solver SAG/SAGA does not cast to float64 [MRG] ENH Ridge with solver SAG/SAGA does not cast to float64 May 3, 2019
@glemaitre glemaitre changed the title [MRG] ENH Ridge with solver SAG/SAGA does not cast to float64 [MRG+1] ENH Ridge with solver SAG/SAGA does not cast to float64 May 3, 2019
@glemaitre
Copy link
Member

@jnothman @GaelVaroquaux I corrected the tests. Do you want to merge for 0.21 or 0.22.
If 0.22, I'll change the what's new.

@jnothman jnothman merged commit 0dac63f into scikit-learn:master May 4, 2019
@jnothman
Copy link
Member

jnothman commented May 4, 2019

I think it should be fine to release, but we will need to start pulling together the list of issues for final release, and choose a date for release.

jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request May 6, 2019
@massich @jnothman
ENH Ridge with solver SAG/SAGA does not cast to float64 (scikit-learn…
7d182ca 
…#13302)
This was referenced May 7, 2019
Merged
Merged
@glemaitre glemaitre mentioned this pull request May 8, 2019
Merged
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@massich @koenvandevelde
ENH Ridge with solver SAG/SAGA does not cast to float64 (scikit-learn…
fc11442 
…#13302)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agramfort agramfort agramfort left review comments

@GaelVaroquaux GaelVaroquaux GaelVaroquaux left review comments

Assignees

@glemaitre glemaitre

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ridge with SAG/SAGA should not cast to float 64
5 participants
@massich @GaelVaroquaux @jnothman @glemaitre @agramfort