MRG : Fixed Precompute=False error in RandomizedLasso. #1856

Closed
wants to merge 6 commits into
from

Conversation

7 participants
@Calvin-O

No description provided.

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Apr 13, 2013

Owner

I don't understand why you don't want to make it possible to precompute the Gram matrix any more when calling lars_path. Can you add a test the demonstrate the unexpected behavior with the old version and that is fixed by your change?

Owner

ogrisel commented Apr 13, 2013

I don't understand why you don't want to make it possible to precompute the Gram matrix any more when calling lars_path. Can you add a test the demonstrate the unexpected behavior with the old version and that is fixed by your change?

@Calvin-O

This comment has been minimized.

Show comment Hide comment
@Calvin-O

Calvin-O Apr 14, 2013

The documentation for RandomizedLasso says that precompute can be True, False, or 'auto'. However, when I tried to explicitly pass False, I got an exception.

The documentation for RandomizedLasso says that precompute can be True, False, or 'auto'. However, when I tried to explicitly pass False, I got an exception.

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Apr 14, 2013

Owner

What exception? BTW any PR for a fix like this requires adding a non regression test to demonstrate the failure that is being resolved by the fix.

Owner

ogrisel commented Apr 14, 2013

What exception? BTW any PR for a fix like this requires adding a non regression test to demonstrate the failure that is being resolved by the fix.

@Calvin-O

This comment has been minimized.

Show comment Hide comment
@Calvin-O

Calvin-O Apr 14, 2013

Look at #1854 . it mentions the same thing( I hadn't seen this or I would have referenced this already) .. would you like me to add a test?

Look at #1854 . it mentions the same thing( I hadn't seen this or I would have referenced this already) .. would you like me to add a test?

@agramfort

This comment has been minimized.

Show comment Hide comment
@agramfort

agramfort Apr 14, 2013

Owner

+1 for a test. I also don't get why we should ignore precompute. There is certainly a problem but I am not convinced it fixes it for all possibilities of precompute.

Owner

agramfort commented Apr 14, 2013

+1 for a test. I also don't get why we should ignore precompute. There is certainly a problem but I am not convinced it fixes it for all possibilities of precompute.

@Calvin-O

This comment has been minimized.

Show comment Hide comment
@Calvin-O

Calvin-O Apr 14, 2013

I passed precompute as an argument in the test_randomized_lasso method and tested it by passing True,False,None and 'auto'.

I passed precompute as an argument in the test_randomized_lasso method and tested it by passing True,False,None and 'auto'.

@ogrisel

This comment has been minimized.

Show comment Hide comment
@ogrisel

ogrisel Apr 15, 2013

Owner

Indeed LassoLarsIC also need more tests to check the various possible values for precompute. But passing precompute=True should not set Gram=None but force it's precomputation instead.

Owner

ogrisel commented Apr 15, 2013

Indeed LassoLarsIC also need more tests to check the various possible values for precompute. But passing precompute=True should not set Gram=None but force it's precomputation instead.

@agramfort

This comment has been minimized.

Show comment Hide comment
@agramfort

agramfort Apr 15, 2013

Owner

make sure your code is pep8

how long is the new test to run?

Owner

agramfort commented Apr 15, 2013

make sure your code is pep8

how long is the new test to run?

@Calvin-O

This comment has been minimized.

Show comment Hide comment
@Calvin-O

Calvin-O Apr 16, 2013

@ogrisel I agree. precompute=True should force the precomputation. Should I add a condition that passes precompute=none in lars_path when precompute is passed as False in RandomizedLasso?

@agramfort The new test is 25 logical lines ( and 44 physical lines).

@ogrisel I agree. precompute=True should force the precomputation. Should I add a condition that passes precompute=none in lars_path when precompute is passed as False in RandomizedLasso?

@agramfort The new test is 25 logical lines ( and 44 physical lines).

@Calvin-O

This comment has been minimized.

Show comment Hide comment
@Calvin-O

Calvin-O Apr 16, 2013

Also, should I add a similar test for LassoLarsIC?

Also, should I add a similar test for LassoLarsIC?

@Calvin-O

This comment has been minimized.

Show comment Hide comment
@Calvin-O

Calvin-O Apr 16, 2013

@ogrisel @agramfort I think I fixed the problem. The Gram can only be passed as None or auto. And so it can not be equated to precompute.
If it is auto it precomputes the matrix from X and if it's None it ignores.
I added a condition if precompute is set to True or auto, then Gram is passed as auto and the precomputation occurs.
However, if precompute is passed as None or False, then Gram is ignored as it should be.
I just checked it on my machine and it passes the test I wrote for various values of precompute. I will pep8 the code and push it here.

@ogrisel @agramfort I think I fixed the problem. The Gram can only be passed as None or auto. And so it can not be equated to precompute.
If it is auto it precomputes the matrix from X and if it's None it ignores.
I added a condition if precompute is set to True or auto, then Gram is passed as auto and the precomputation occurs.
However, if precompute is passed as None or False, then Gram is ignored as it should be.
I just checked it on my machine and it passes the test I wrote for various values of precompute. I will pep8 the code and push it here.

The Gram can only be passed as None or auto. And so it can not be equ…
…ated to precompute.

If it is auto it precomputes the matrix from X and if it's None it ignores.
@jnothman

This comment has been minimized.

Show comment Hide comment
@jnothman

jnothman May 12, 2013

please add spaces around the ==

please add spaces around the ==

@amueller amueller added the Bug label Jan 23, 2015

@amueller amueller added this to the 0.17 milestone Aug 25, 2015

@lesteve lesteve modified the milestones: 0.17, 0.18 Jul 27, 2016

@amueller amueller modified the milestones: 0.18, 0.19 Sep 22, 2016

@amueller

This comment has been minimized.

Show comment Hide comment
@amueller

amueller Oct 7, 2016

Owner

Can you please rebase?

Owner

amueller commented Oct 7, 2016

Can you please rebase?

@raghavrv raghavrv removed the Needs Review label Nov 4, 2016

@agramfort

This comment has been minimized.

Show comment Hide comment
@agramfort

agramfort Jun 6, 2017

Owner

randomized_l1 will be removed. CLosing

Owner

agramfort commented Jun 6, 2017

randomized_l1 will be removed. CLosing

@agramfort agramfort closed this Jun 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment