8000 Lasso error when precompute='auto' · Issue #19283 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

Lasso error when precompute='auto' #19283

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.

Sign up for GitHub

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

Jump to bottom
Closed
blthayer opened this issue Jan 26, 2021 · 6 comments · Fixed by #19412
Closed

Lasso error when precompute='auto' #19283

blthayer opened this issue Jan 26, 2021 · 6 comments · Fixed by #19412
Labels
module:linear_model

Comments

@blthayer
Copy link

Describe the bug

The docs state that the precompute parameter can be "‘auto’, bool or array-like of shape (n_features, n_features)." However, attempting to use precompute='auto' leads to a ValueError.

Steps/Code to Reproduce

from sklearn.linear_model import Lasso

model = Lasso(precompute='auto')
model.fit([[1, 2], [3, 4]], [[1], [3]])

Expected Results

No error, should "just work."

Actual Results

Traceback (most recent call last):
  File "/opt/project/vpf/elm/tmp.py", line 4, in <module>
    model.fit([[1, 2], [3, 4]], [[1], [3]])
  File "/usr/local/lib/python3.8/site-packages/sklearn/linear_model/_coordinate_descent.py", line 757, in fit
    raise ValueError('precompute should be one of True, False or'
ValueError: precompute should be one of True, False or array-like. Got 'auto'

Versions

Output from sklearn.show_versions():

System:
    python: 3.8.6 (default, Nov 18 2020, 13:49:49)  [GCC 8.3.0]
executable: /usr/local/bin/python
   machine: Linux-4.19.104-microsoft-standard-x86_64-with-glibc2.2.5
Python dependencies:
          pip: 20.3.3
   setuptools: 50.3.2
      sklearn: 0.24.0
        numpy: 1.19.5
        scipy: 1.6.0
       Cython: None
       pandas: None
   matplotlib: None
       joblib: 1.0.0
threadpoolctl: 2.1.0
Built with OpenMP: True
@jjerphan
Copy link
Member
jjerphan commented Feb 2, 2021 •
edited
Loading

I can reproduce. This bug does not exist for the other models supporting precompute:

import numpy as np
from sklearn.linear_model import (
    Lasso,
    LassoCV,
    ElasticNetCV,
)

models = [
    Lasso,
    LassoCV,
    ElasticNetCV,
]

if __name__ == "__main__":
    X = np.random.rand(10, 2)
    y = np.random.rand(10)

    for Model in models:
        try:
            model = Model(precompute='auto')
            model.fit(X, y)
            print("✅", Model)
        except Exception as e:
            print("❌", Model)
            print(type(e), ":", e)
        print()
❌ <class 'sklearn.linear_model._coordinate_descent.Lasso'>
<class 'ValueError'> : precompute should be one of True, False or array-like. Got 'auto'

✅ <class 'sklearn.linear_model._coordinate_descent.LassoCV'>

✅ <class 'sklearn.linear_model._coordinate_descent.ElasticNetCV'>

@anton-khimich
Copy link
anton-khimich commented Feb 2, 2021 •
edited
Loading

Lasso inherits fit from the class ElasticNet which does not allow for precompute='auto'. #11014 and #14591 touch upon something similar, so the solution may be to overload fit in Lasso to set precompute to False if it is 'auto' before calling super.fit().

It's also mentioned that the 'auto' function for Lasso has been removed although I do not know if that is actually the case seeing as the documentation still mentions it.

@jjerphan
Copy link
Member
jjerphan commented Feb 2, 2021 •
edited
Loading

One could use a private method to specify the check on precompute, which would be called inside ElasticNet.fit and which would be overwritten in. Lasso.

@jjerphan
Copy link
Member
jjerphan commented Feb 3, 2021

Lasso inherits fit from the class ElasticNet which does not allow for precompute='auto'. #11014 and #14591 touch upon something similar [...]

Thanks for the pointers. #3249 might be of relevance.

[...] so the solution may be to overload fit in Lasso to set precompute to False if it is 'auto' before calling super.fit().

Yes, alternatively there was another proposition like the one proposed by @agramfort in #14591 to set in the case of 'auto':

 precompute = n_samples > K * n_features

where K could be set according to benchmark.

@amueller, @agramfort: since you've worked on similar topics (#11014, #14591), what would you suggestions be regarding supporting for precompute='auto' for both Lasso and ElasticNet?

@agramfort
Copy link
Member

AFAIK the precompute 'auto' in Lasso was removed as it was not clear when to toggle True or False. We decided to let the user decide and use True by default as it's the best default. I would not put it back unless there is a thorough benchmark (but I think it's hard).

@jjerphan
Copy link
Member
jjerphan commented Feb 9, 2021

AFAIK the precompute 'auto' in Lasso was removed as it was not clear when to toggle True or False. We decided to let the user decide and use True by default as it's the best default. I would not put it back unless there is a thorough benchmark (but I think it's hard).

Thanks for the details. It looks like precompute was set to False to handle sparsity: #19348 offers some clarification.

I would just remove the mentions of 'auto' in the docstring as its support is not present and not relevant.

@jjerphan jjerphan mentioned this issue Feb 9, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
module:linear_model
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

DOC Explicitly remove support for precompute='auto' for Lasso jjerphan/scikit-learn
5 participants
@agramfort @cmarmo @jjerphan @blthayer @anton-khimich
0