Skip to content

Refactor linear model coordinate descent code to work as LARS for cv#2186

Closed
agramfort wants to merge 3 commits intoscikit-learn:masterfrom
agramfort:refactor_cd_cv
Closed

Refactor linear model coordinate descent code to work as LARS for cv#2186
agramfort wants to merge 3 commits intoscikit-learn:masterfrom
agramfort:refactor_cd_cv

Conversation

@agramfort
Copy link
Copy Markdown
Member

@agramfort agramfort commented Jul 22, 2013

there is a million dollar bug cf. plot_model_lasso_model_selection.py

tests pass but the example output is wrong...

@agramfort agramfort mentioned this pull request Jul 22, 2013
Closed
@jaquesgrobler
Copy link
Copy Markdown
Member

jaquesgrobler commented Jul 22, 2013

i'll look at this when i'm home a bit too.

2013/7/22, Alexandre Gramfort notifications@github.com:

there is a million dollar bug cf. plot_model_lasso_model_selection.py

tests pass but the example output is wrong...
You can merge this Pull Request by running:

git pull https://github.com/agramfort/scikit-learn refactor_cd_cv

Or you can view, comment on it, or merge it online at:

#2186

-- Commit Summary --

  • make new classes for lasso_path/enet_path and deprecate old
  • lasso_path output changed/depr warnings temporarily removed
  • lasso_path output changed/depr warnings temporarily removed
  • old_return added and tests adjusted
  • slight cleanup in tests
  • squashed:example cleanup, warning change, Alex`s suggestions
  • fix broken test
  • cosmetic: same line parameters
  • remove ugly divider from example
  • clarify test description and remove duplicate warning
  • update what`s new
  • fix merge artifact - log10s readded
  • fix merge artifact
  • comments and pep8 changes
  • remove unneccasary if
  • misc
  • API : deprecating fit_intercept and normalize in lasso_path and
    enet_path
  • ENH : more cleanup on linear models path refactoring
  • API : massive refactoring of CV models in coordinate descent. Now the
    algo core is in path functions
  • attempt to fix plot_lasso_model_selection

-- File Changes --

M doc/whats_new.rst (6)
M examples/linear_model/plot_lasso_coordinate_descent_path.py (47)
M examples/linear_model/plot_lasso_model_selection.py (2)
M sklearn/linear_model/base.py (7)
M sklearn/linear_model/coordinate_descent.py (1261)
M sklearn/linear_model/tests/test_coordinate_descent.py (42)
M sklearn/linear_model/tests/test_least_angle.py (16)
M sklearn/linear_model/tests/test_sparse_coordinate_descent.py (5)

-- Patch Links --

https://github.com/scikit-learn/scikit-learn/pull/2186.patch
https://github.com/scikit-learn/scikit-learn/pull/2186.diff

@agramfort
Copy link
Copy Markdown
Member Author

agramfort commented Jul 22, 2013

@fabianp spotted a bug with precompute.

@agramfort agramfort mentioned this pull request Jul 22, 2013
Closed
2 tasks
vene
vene reviewed Jul 23, 2013
View reviewed changes
sklearn/linear_model/coordinate_descent.py
Copy link
Copy Markdown
Member

@vene vene Jul 23, 2013

Choose a reason for hiding this comment

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

Could you point me to where the decision to deprecate has been made? Can we also deprecate normalize in orthogonal matching pursuit? The code is on the point of unmaintainability because of branching in case of normalization and copy_X, copy_Gram, copy_Xy interaction.

Copy link
Copy Markdown
Member Author

@agramfort agramfort Jul 23, 2013

Choose a reason for hiding this comment

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

Could you point me to where the decision to deprecate has been made?

search for :

  • if normalize is not None:
  •    warnings.warn("normalize param will be removed in 0.15."

  •                  " Intercept fitting and feature normalization will be"

  •                  " done in estimators.",

  •                  DeprecationWarning, stacklevel=2)

Can
we also deprecate normalize in orthogonal matching pursuit? The code is on
the point of unmaintainability because of branching in case of normalization
and copy_X, copy_Gram, copy_Xy interaction.

Yes I think it's the most reasonable option. Indeed all these options make
it a nightmare to maintain.

@agramfort
Copy link
Copy Markdown
Member Author

agramfort commented Jul 23, 2013

cleanup, rebased, ready for review !

warning the diff is horrible as many functions have moved. It's a big refactoring.

@agramfort
Copy link
Copy Markdown
Member Author

agramfort commented Jul 23, 2013

travis is happy !

@fabianp
Copy link
Copy Markdown
Member

fabianp commented Jul 23, 2013

Merged by rebase

@fabianp fabianp closed this Jul 23, 2013
@arjoly arjoly mentioned this pull request Jul 26, 2013
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@agramfort @jaquesgrobler @fabianp @vene