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

[MRG+2] temporary fix for sparse ridge with intercept fitting #5360

Merged
merged 2 commits into from Oct 13, 2015

Conversation

Projects
None yet
6 participants
@TomDLT
Member

TomDLT commented Oct 7, 2015

Temporary fix for #4710, using 'sag' solver, that can directly fit the intercept.

If fit_intercept=True and X is sparse, then the solver is switch to 'sag'.

I am aware this is not what we want for long term, yet it is a temporary fix for release 0.17.

@TomDLT TomDLT added this to the 0.17 milestone Oct 7, 2015

@TomDLT TomDLT changed the title from [WIP] temporary fix for sparse ridge with intercept fitting to [MRG] temporary fix for sparse ridge with intercept fitting Oct 7, 2015

@@ -281,6 +286,12 @@ def ridge_regression(X, y, alpha, sample_weight=None, solver='auto',
-----
This function won't compute the intercept.
"""
if return_intercept:

This comment has been minimized.

@agramfort

agramfort Oct 7, 2015

Member

where do you check X is sparse?

This comment has been minimized.

@TomDLT

TomDLT Oct 9, 2015

Member

in the fit(), here

This comment has been minimized.

@amueller

amueller Oct 9, 2015

Member

and solver != "sag" right?

This comment has been minimized.

@TomDLT

TomDLT Oct 9, 2015

Member

Well, 'sag' currently uses the same centering trick than other solvers to avoid computing the intercept.
The intercept is computed with 'sag' only if the data has not been centered.

This comment has been minimized.

@TomDLT

TomDLT Oct 9, 2015

Member

Ok I got your point, I'll add it

y = np.dot(X, w) + i
X_csr = sp.csr_matrix(X)
dense = Ridge(alpha=1., tol=1.e-15, solver='sag', fit_intercept=True)

This comment has been minimized.

@amueller

amueller Oct 9, 2015

Member

is there a check that the solution to the dense model doesn't depend on the solver? (I hope so).

This comment has been minimized.

@TomDLT

TomDLT Oct 9, 2015

Member

 test_ridge_individual_penalties does check it indirectly, right?

@amueller

This comment has been minimized.

Member

amueller commented Oct 9, 2015

needs a whatsnew, otherwise looks good apart from minor comments.

def test_ridge_fit_intercept_sparse():
r = np.random.RandomState(42)

This comment has been minimized.

@amueller

amueller Oct 9, 2015

Member

or make_regression(...) + 10, right?

This comment has been minimized.

@TomDLT

TomDLT Oct 9, 2015

Member

Sure :) I was tired

X_csr = sp.csr_matrix(X)
dense = Ridge(alpha=1., tol=1.e-15, solver='sag', fit_intercept=True)
sparse = Ridge(alpha=1., tol=1.e-15, solver='sag', fit_intercept=True)

This comment has been minimized.

@ogrisel

ogrisel Oct 12, 2015

Member

Could you please also add the case where solver != 'sag' and check for the solver switch warning with assert_warns?

This comment has been minimized.

@TomDLT
@ogrisel

This comment has been minimized.

Member

ogrisel commented Oct 12, 2015

Apart from my last comment and a needed rebase on top of master, +1 for merge as well.

@ogrisel ogrisel changed the title from [MRG] temporary fix for sparse ridge with intercept fitting to [MRG+2] temporary fix for sparse ridge with intercept fitting Oct 12, 2015

@amueller

This comment has been minimized.

Member

amueller commented Oct 12, 2015

maybe @mblondel wants to do a quick look-over?

warnings.warn("In Ridge, only 'sag' solver can currently fit the "
"intercept when X is sparse. Solver has been "
"automatically changed into 'sag'.")
solver = 'sag'

This comment has been minimized.

@mblondel

mblondel Oct 13, 2015

Member

Doesn't this raise a warning even when the data isn't sparse?

This comment has been minimized.

@TomDLT

TomDLT Oct 13, 2015

Member

You are right, I did not think about the public use of ridge_regression.
I'll change that

if return_n_iter and return_intercept:
return coef, n_iter, intercept
elif return_intercept:
return coef, intercept

This comment has been minimized.

@mblondel

mblondel Oct 13, 2015

Member

You didn't document the return in the docstring.

This comment has been minimized.

@TomDLT

TomDLT Oct 13, 2015

Member

Thanks, I'll change that

@mblondel

This comment has been minimized.

Member

mblondel commented Oct 13, 2015

LGTM once my comments are addressed.

In #4710, I gave another temporary quick fix which can be used with all solvers. It could be worth investing it if time permits.

@TomDLT

This comment has been minimized.

Member

TomDLT commented Oct 13, 2015

In #4710, I gave another temporary quick fix which can be used with all solvers. It could be worth investing it if time permits.

I chose a quicker fix for the release of v0.17, but your suggestion might be better for long term.

TomDLT added a commit that referenced this pull request Oct 13, 2015

Merge pull request #5360 from TomDLT/sparseridge
[MRG+2] temporary fix for sparse ridge with intercept fitting

@TomDLT TomDLT merged commit 90922ea into scikit-learn:master Oct 13, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@TomDLT TomDLT deleted the TomDLT:sparseridge branch Oct 16, 2015

@giorgiop

This comment has been minimized.

Contributor

giorgiop commented Oct 23, 2015

I have noticed that the warning for sparse input is now in contrast with the dosctring.

@amueller

This comment has been minimized.

Member

amueller commented Oct 23, 2015

PR welcome @giorgiop ;)

@amueller

This comment has been minimized.

Member

amueller commented Oct 23, 2015

[or open an issue please?]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment