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+1] Ensure coef_ is an ndarray when fitting LassoLars #8160

Merged
merged 4 commits into from Jan 18, 2017

Conversation

@perimosocordiae
Copy link
Contributor

@perimosocordiae perimosocordiae commented Jan 5, 2017

This PR fixes #1615, an issue related to fitting a LassoLars model with the specific combination of fit_path=True, fit_intercept=False, and >1 targets.

The bug was masked in the fit_intercept=True case because of this line,
in which the coefficient list is promoted to an ndarray by division with another ndarray.

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 6, 2017

Great, it looks like you've got most of the way to a regression test at #1615. Please add it?

@perimosocordiae
Copy link
Contributor Author

@perimosocordiae perimosocordiae commented Jan 6, 2017

Tests added.

Copy link
Member

@jnothman jnothman left a comment

LGTM, thanks!

@jnothman jnothman changed the title [MRG] Ensure coef_ is an ndarray when fitting LassoLars [MRG+1] Ensure coef_ is an ndarray when fitting LassoLars Jan 7, 2017
@jnothman
Copy link
Member

@jnothman jnothman commented Jan 7, 2017

(I've confirmed the test fails in master)

@@ -689,6 +689,8 @@ def fit(self, X, y, Xy=None):
a[0] for a in (self.alphas_, self.active_, self.coef_path_,
self.coef_)]
self.n_iter_ = self.n_iter_[0]
else:
self.coef_ = np.array(self.coef_)

This comment has been minimized.

@agramfort

agramfort Jan 7, 2017
Member

it would be more efficient to preallocate self.coef_ to an empty array rather than first having a list than copying the data to an array

This comment has been minimized.

@perimosocordiae

perimosocordiae Jan 10, 2017
Author Contributor

True. Would you like me to add that optimization to this PR?

This comment has been minimized.

@tguillemot

tguillemot Jan 16, 2017
Contributor

If you can do it, it will be great :)

Copy link
Contributor

@tguillemot tguillemot left a comment

+1 if tests are ok.

Copy link
Member

@jnothman jnothman left a comment

a changelog entry would be great, otherwise let's merge.

@agramfort
Copy link
Member

@agramfort agramfort commented Jan 17, 2017

LGTM

see why CIs complain but diff looks clean

@tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Jan 17, 2017

@agramfort Error on circleci are not related. We have the same problem on several PR (ex : #8135).

@tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Jan 18, 2017

@perimosocordiae Ci problems seems solved now, can you rebase ?

@jnothman
Copy link
Member

@jnothman jnothman commented Jan 18, 2017

Nah. Merging.

@jnothman jnothman merged commit 08772c4 into scikit-learn:master Jan 18, 2017
1 of 3 checks passed
1 of 3 checks passed
ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Jan 18, 2017

@jnothman Thanks :)

@tguillemot
Copy link
Contributor

@tguillemot tguillemot commented Jan 18, 2017

@perimosocordiae Thanks for your work

sergeyf added a commit to sergeyf/scikit-learn that referenced this pull request Feb 28, 2017
@Przemo10 Przemo10 mentioned this pull request Mar 17, 2017
Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
NelleV added a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants