Skip to content

Conversation

Calvin-O
Copy link

No description provided.

@ogrisel
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

+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
Copy link
Author

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

@ogrisel
Copy link
Member

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
Copy link
Member

make sure your code is pep8

how long is the new test to run?

@Calvin-O
Copy link
Author

@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
Copy link
Author

Also, should I add a similar test for LassoLarsIC?

@Calvin-O
Copy link
Author

@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.

…ated to precompute.

If it is auto it precomputes the matrix from X and if it's None it ignores.
@amueller amueller removed this from the 0.18 milestone Sep 22, 2016
@amueller
Copy link
Member

amueller commented Oct 7, 2016

Can you please rebase?

@agramfort
Copy link
Member

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants