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

[MRG] Implement fitting intercept with `sparse_cg` solver in Ridge regression #13336

Merged
merged 13 commits into from Mar 1, 2019

Conversation

4 participants
@btel
Copy link
Contributor

commented Feb 28, 2019

Reference Issues/PRs

See also #470

It follows the same trick as introduced in PR #13279 by @agramfort

What does this implement/fix? Explain your changes.

This implements fitting intercept with sparse_cg solver in Ridge regression (i.e. when fit_intercept==True) for sparse inputs. It also means that both sparse and dense cases give the same result.

Any other comments?

Important: This PR changes the auto-selected solver (solver='auto') from sag to sparse_cg when fit_intercept==True and input is sparse.

There are still problems with the code:

  1. 'auto' mode in ridge_regression function may trigger wrong solvers (for example, when inputs is sparse and sample_weight is passed)

  2. Warning message about changing the solver is not fully informative/correct. For example, user might choose the sparse_cg solver instead of going the sag way.

  3. The estimator object is not informed about the fact that the solver was changed (or which solver was selected by auto)

They are not related to this PR and will be fixed in another PR.

@glemaitre
Copy link
Contributor

left a comment

@btel Could you add an entry in what's new as well.

Show resolved Hide resolved sklearn/linear_model/ridge.py Outdated
Show resolved Hide resolved sklearn/linear_model/ridge.py Outdated

@btel btel force-pushed the btel:fit_intercept_ridge_sparse_cg branch from 584709b to 86248d1 Feb 28, 2019

@agramfort agramfort added this to In progress in Sprint Paris 2019 Feb 28, 2019

Show resolved Hide resolved sklearn/linear_model/ridge.py Outdated
@agramfort

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@btel you have pep8 errors cf. circle lint

@btel btel force-pushed the btel:fit_intercept_ridge_sparse_cg branch from 1b8a2e8 to c3c6e8f Mar 1, 2019

Bartosz Telenczuk added some commits Mar 1, 2019

Bartosz Telenczuk
@GaelVaroquaux
Copy link
Member

left a comment

LGTM (although the addition of _ridge_regression is ugly).

+1 for merge.

Merging.

@GaelVaroquaux GaelVaroquaux merged commit c8e757d into scikit-learn:master Mar 1, 2019

14 checks passed

LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
scikit-learn.scikit-learn Build #20190301.97 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_np_atlas) Linux py35_np_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda) Linux pylatest_conda succeeded
Details
scikit-learn.scikit-learn (Windows py35_32) Windows py35_32 succeeded
Details
scikit-learn.scikit-learn (Windows py37_64) Windows py37_64 succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda) macOS pylatest_conda succeeded
Details

Sprint Paris 2019 automation moved this from In progress to Done Mar 1, 2019

@btel btel deleted the btel:fit_intercept_ridge_sparse_cg branch Mar 1, 2019

@btel

This comment has been minimized.

Copy link
Contributor Author

commented Mar 1, 2019

LGTM (although the addition of _ridge_regression is ugly).

@GaelVaroquaux I am happy to discuss/implement prettier solutions for propagating X_scale and X_offset from Ridge.fit method.

@agramfort

This comment has been minimized.

Copy link
Member

commented Mar 2, 2019

@btel

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2019

@agramfort sounds good, but it will require a good deal of refactoring.

xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019

[MRG] Implement fitting intercept with `sparse_cg` solver in Ridge re…
…gression (scikit-learn#13336)

* add skeleton for fit_intercept with sparse_cg

* fix sparse_cg solver with fit_intercept=True

* fix test

* linting

* add what's new entry

* remove X_scale and X_offset from public interface of ridge_regression

* reformat if clause

* fixed linting issues

* add comments on about the conditions of different code branches

* update warning

* remove whitespace

* add extra checks in the test of ridge with fit_intercept

* remove unused argument

xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019

xhlulu added a commit to xhlulu/scikit-learn that referenced this pull request Apr 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.