MRG - ENH change lasso_path output to lars_path style by jaquesgrobler · Pull Request #1947 · scikit-learn/scikit-learn

jaquesgrobler · 2013-05-07T16:04:08Z

Addresses Issue #32

~~Almost done..~~

Just an example update, check some docstrings and go over tests left - Then
I'll change to MRG.

Unless I'm missing anything, I'd say this guy is ready.

jaquesgrobler · 2013-05-07T16:05:07Z

Will also have to fix conflicts and rebase 😬

agramfort · 2013-05-13T13:05:51Z

sklearn/linear_model/coordinate_descent.py

can you have a more explicit name than old_return? smtg like return_models

hey Alex.. thanks for review. I'd done most of these already but I can't seem to push commits right now (whole afternoon it's been impossible). Hopefully I'll get the your suggestions up just now..
return_models sounds good to me. I'll make that so.
This warning is also gonna be replaced with a if(..) type warning as this way it's annoying and appears regardless of your choice of 'old_return'.. Thanks again :)

agramfort · 2013-05-13T13:11:10Z

also all the calls to lasso_path in docs and functions should now use the old_return=False or the better name suggested.

jaquesgrobler · 2013-05-13T19:23:27Z

Thanks @agramfort ! I'll push your sugested changes when my connection stops it #$@% :) haha

jaquesgrobler · 2013-05-13T20:01:17Z

finally 😠 haha

@agramfort you think it's necessary to change the benchmarks to return_models=False? Since the output doesn't get used, would it make a difference to set to so?

Also I think I've got all the docstrings ect covered.

jaquesgrobler · 2013-05-14T08:32:54Z

Rebased on master

jaquesgrobler · 2013-05-22T14:03:07Z

Changed from WIP to MRG - waiting on Travis :)

GaelVaroquaux · 2013-05-23T05:52:45Z

examples/linear_model/plot_lasso_coordinate_descent_path.py

Is there a reason why the 'log10(alphas)' disappeared, or is this a merge artifact (the remark applies to the whole file).

I think it came from the merge.. I don't recall any reason for removing it.
I'll switch it back . Thanks for pointing out 👍

I just found another merge related bug due to this one.. fixing

jaquesgrobler · 2013-05-23T17:52:24Z

Ready

jaquesgrobler · 2013-06-06T10:56:11Z

I'm gonna merge this in the next hour if nobody objects,

mkay?

agramfort · 2013-06-06T19:48:39Z

sklearn/linear_model/coordinate_descent.py

is this necessary?

you have above:

if precompute == 'auto': precompute = (n_samples > n_features)

Will fix, thanks!

agramfort · 2013-06-06T19:54:03Z

to deprecate this feature it should disappear from the code base eg in:

l882: models_train = path(X_train, y[train], **path_params)

then you'll need to take care about the predict especially the use of the intercept if fit_intercept=True

GaelVaroquaux · 2013-06-07T21:02:38Z

examples/linear_model/plot_lasso_coordinate_descent_path.py

I think that you want to add a comment here on the purpose of 'return_models'.

GaelVaroquaux · 2013-06-07T21:03:23Z

examples/linear_model/plot_lasso_coordinate_descent_path.py

Missing whitespace after coma to be PEP8

GaelVaroquaux · 2013-06-07T21:11:34Z

I left a couple of very minor comments. After that, please do merge!

agramfort · 2013-06-09T09:14:20Z

to clarify my previous comment. The models are used in LinearModelCV so
LassoCV and ElasticNetCV. Especially their predict methods are used taking
into account the intercepts. Before deprecating return_models=True we need
to replace the use of models everywhere.

GaelVaroquaux · 2013-06-09T19:32:08Z

The models are used in LinearModelCV so LassoCV and ElasticNetCV.
Especially their predict methods are used taking into account the
intercepts. Before deprecating return_models=True we need to replace
the use of models everywhere.

Indeed, this is blocker for the merge. Also, it will lead to
difficulties, as the current API does not return the intercept. We might
have to modify it to either add the intercept as an additional return
argument, or included it in the coef vector (which then becomes of
dimensions n_features + 1).

jaquesgrobler · 2013-06-21T12:34:30Z

I've addressed all the smaller comments.. however this last one is a bit of a problem.. i guess it would be a bit sloppy to leave 'return_models' in as is indefinitely.. I'm not sure which solution is better regarding changing the API and the intercepts. Since my inria report and presentation is now done after consuming my whole week, I could maybe have a look.. Any guidance appreciated :)

agramfort · 2013-07-22T07:22:56Z

I am starting with this !

jaquesgrobler · 2013-07-22T08:27:38Z

Cool - keep me posted 👍

agramfort · 2013-07-22T15:24:44Z

closing this in favor of #2186

agramfort reviewed May 13, 2013
View reviewed changes

jaquesgrobler added 8 commits May 14, 2013 01:59

make new classes for lasso_path/enet_path and deprecate old

0c9dda1

lasso_path output changed/depr warnings temporarily removed

79b2e39

lasso_path output changed/depr warnings temporarily removed

6fb8c01

old_return added and tests adjusted

1b41724

slight cleanup in tests

8e1cf58

squashed:example cleanup, warning change, Alex`s suggestions

c69fdfb

fix broken test

cfb6219

cosmetic: same line parameters

bf8ffe0

jaquesgrobler added 2 commits May 22, 2013 15:14

remove ugly divider from example

e871943

clarify test description and remove duplicate warning

d12557d

update what`s new

c0bfeb4

GaelVaroquaux reviewed May 23, 2013
View reviewed changes

jaquesgrobler added 2 commits May 23, 2013 10:21

fix merge artifact - log10s readded

0f1c822

fix merge artifact

6d21852

GaelVaroquaux mentioned this pull request May 26, 2013

[WIP] Estimator.iter_fits for efficient parameter search #2000

Closed

agramfort reviewed Jun 6, 2013
View reviewed changes

GaelVaroquaux reviewed Jun 7, 2013
View reviewed changes

examples/linear_model/plot_lasso_coordinate_descent_path.py Outdated

Copy link
Copy Markdown

Member

GaelVaroquaux Jun 7, 2013

Choose a reason for hiding this comment

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

Missing whitespace after coma to be PEP8

jaquesgrobler added 2 commits June 21, 2013 14:35

comments and pep8 changes

aab8545

remove unneccasary if

8020192

agramfort closed this Jul 22, 2013

Uh oh!

Conversation

jaquesgrobler commented May 7, 2013

Uh oh!

jaquesgrobler commented May 7, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented May 13, 2013

Uh oh!

jaquesgrobler commented May 13, 2013

Uh oh!

jaquesgrobler commented May 13, 2013

Uh oh!

jaquesgrobler commented May 14, 2013

Uh oh!

jaquesgrobler commented May 22, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaquesgrobler commented May 23, 2013

Uh oh!

jaquesgrobler commented Jun 6, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agramfort commented Jun 6, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Jun 7, 2013

Uh oh!

agramfort commented Jun 9, 2013

Uh oh!

GaelVaroquaux commented Jun 9, 2013

Uh oh!

jaquesgrobler commented Jun 21, 2013

Uh oh!

agramfort commented Jul 22, 2013

Uh oh!

jaquesgrobler commented Jul 22, 2013

Uh oh!

agramfort commented Jul 22, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants