-
-
Notifications
You must be signed in to change notification settings - Fork 26.2k
MRG : Fixed Precompute=False error in RandomizedLasso. #1856
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
Conversation
…sed in lars_path.
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? |
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. |
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. |
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? |
+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. |
I passed precompute as an argument in the test_randomized_lasso method and tested it by passing True,False,None and 'auto'. |
Indeed |
make sure your code is pep8 how long is the new test to run? |
@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). |
Also, should I add a similar test for LassoLarsIC? |
@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. |
…ated to precompute. If it is auto it precomputes the matrix from X and if it's None it ignores.
Can you please rebase? |
randomized_l1 will be removed. CLosing |
No description provided.