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] Fix precomputation of Gram matrix in Lars #5359

Merged
merged 1 commit into from Jun 13, 2017

Conversation

@TomDLT
Copy link
Member

@TomDLT TomDLT commented Oct 7, 2015

This is a fix for the bug reported in #1856.
The parameter precompute (in RandomizedLasso, Lars, LarsLasso, LarsCV and LarsLassoCV) were not consistent and some values proposed in the docstrings (False, array) could raise errors.

This PR includes the following steps:

  • improve lars_path, to allow Gram=True and Gram=False.
  • change the _get_gram method, to handle precompute in the same way in every class.
  • add a warning when an array is given in precompute, in LarsCV and LarsLassoCV.
  • update the docstring.
  • add some tests. test_randomized_l1.py is also updated to be shorter (from 1.582s to 0.817s). test_least_angle.py goes from 1.305s to 1.465s.
@TomDLT TomDLT force-pushed the TomDLT:lars branch from bf4aa3d to 68d0abd Oct 7, 2015
# As we use cross-validation, the Gram matrix is not precomputed here
Gram = self.precompute
if hasattr(Gram, '__array__'):
warnings.warn("Parameter 'precompute' cannot be an array in "

This comment has been minimized.

@arthurmensch

arthurmensch Oct 7, 2015
Contributor

I would say raise an error instead of a warning but it depends on how much we want to nurse the user.

This comment has been minimized.

@TomDLT

TomDLT Oct 7, 2015
Author Member

I was thinking about avoiding an error when a user changes Lars(precompute=G).fit(X, y) into LarsCV(precompute=G).fit(X, y). But it is kind of nursing.

precompute=precompute, n_resampling=n_resampling)
feature_scores_2 = clf.fit(X, y).scores_
assert_array_equal(feature_scores_1, feature_scores_2)

This comment has been minimized.

@arthurmensch

arthurmensch Oct 7, 2015
Contributor

Do we need test for precompute = array_like ?

This comment has been minimized.

@TomDLT

TomDLT Oct 7, 2015
Author Member

In Randomized lasso, precompute = array_like is used only for the computation of alpha when alpha='aic' or 'bic'.
But I can definitely test it. (Done)

@TomDLT TomDLT added this to the 0.17 milestone Oct 7, 2015
@TomDLT TomDLT force-pushed the TomDLT:lars branch from cc0dafd to fac3cbb Oct 7, 2015
@TomDLT TomDLT changed the title [WIP] Fix precomputation of Gram matrix in Lars [MRG] Fix precomputation of Gram matrix in Lars Oct 7, 2015
@TomDLT TomDLT force-pushed the TomDLT:lars branch 2 times, most recently from 0d8ed92 to e059923 Oct 9, 2015
@TomDLT TomDLT force-pushed the TomDLT:lars branch 2 times, most recently from 9d58692 to ed10cf1 Oct 21, 2015
@TomDLT TomDLT force-pushed the TomDLT:lars branch from ed10cf1 to 4d988d3 Nov 2, 2015
@TomDLT TomDLT force-pushed the TomDLT:lars branch from 4d988d3 to 6af1d84 Nov 10, 2015
@TomDLT TomDLT removed this from the 0.17 milestone Dec 5, 2015
@TomDLT TomDLT force-pushed the TomDLT:lars branch from 6af1d84 to 48f94f1 Apr 26, 2016
@agramfort
Copy link
Member

@agramfort agramfort commented Jun 6, 2017

LGTM

@TomDLT can you update what's new to document the bug fix?

@TomDLT
Copy link
Member Author

@TomDLT TomDLT commented Jun 6, 2017

done

@agramfort
Copy link
Member

@agramfort agramfort commented Jun 6, 2017

+1 for MRG when CIs are happy

thx @TomDLT

Copy link
Contributor

@arthurmensch arthurmensch left a comment

To me it would make more sense to explicitely feed the gram matrix in the .fit but I reckon this has little usage anyway. LGTM

return Gram
def _get_gram(self, precompute, X, y):
if (not hasattr(precompute, '__array__')) and (
(precompute is True) or

This comment has been minimized.

@arthurmensch

arthurmensch Jun 6, 2017
Contributor

Can be simplified ?

@agramfort agramfort changed the title [MRG] Fix precomputation of Gram matrix in Lars [MRG+1] Fix precomputation of Gram matrix in Lars Jun 7, 2017
@agramfort
Copy link
Member

@agramfort agramfort commented Jun 7, 2017

CIs are happy here

@agramfort
Copy link
Member

@agramfort agramfort commented Jun 7, 2017

@TomDLT you need to rebase

@TomDLT TomDLT force-pushed the TomDLT:lars branch from eb3badd to b14dd6c Jun 7, 2017
@TomDLT
Copy link
Member Author

@TomDLT TomDLT commented Jun 9, 2017

All checks have passed

@agramfort
Copy link
Member

@agramfort agramfort commented Jun 10, 2017

@TomDLT please fix conflicts here and then let's merge.

@TomDLT TomDLT force-pushed the TomDLT:lars branch 2 times, most recently from f7ff310 to 71f14cd Jun 12, 2017
@TomDLT TomDLT force-pushed the TomDLT:lars branch from 71f14cd to f598858 Jun 12, 2017
@TomDLT
Copy link
Member Author

@TomDLT TomDLT commented Jun 13, 2017

The AppVeyor failures are on master #9111

@agramfort agramfort merged commit bb1299b into scikit-learn:master Jun 13, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@agramfort
Copy link
Member

@agramfort agramfort commented Jun 13, 2017

thanks @TomDLT

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017
dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
dmohns added a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 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
AishwaryaRK added a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@TomDLT TomDLT deleted the TomDLT:lars branch Jun 15, 2018
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.

None yet

4 participants
You can’t perform that action at this time.