Repository: scikit-learn/scikit-learn
Pull request: Deprecate lar positive
#### Reference Issues/PRs

Closes #9837 

#### What does this implement/fix? Explain your changes.

Deprecate option posistive=True for Lars model.

#### Any other comments?

it was reported as broken and I could not find an easy bug fix.
I propose to deprecate and suggest to use Lasso model for which it works.

Problem statement:
Non-negative coefficients with lars
I am running this piece of code

```py
import numpy as np
from sklearn.linear_model import Lars
import sklearn

flag = True
print sklearn.__version__
while flag:
    X = np.random.randn(10,5)
    Y = np.random.randn(10)
    l=Lars(fit_intercept=False, positive=True)

    l.fit(X,Y)
    print l.coef_
    for i in l.coef_ :
        if i < 0 :
            flag = False

```

I am getting the following output

0.19.0
[  0.00000000e+00   0.00000000e+00   5.21722148e-01   3.10743837e-01
   4.18778571e-04]
[ 0.          0.          0.          0.45541234  0.09542552]
**[ 0.80960144  0.35327211  0.17399586  0.16972765 -0.02813728]**

with negative coefficients

Hints from issue discussion:
thanks for the report. please make this do np.random.seed(i) for iteration
i, and report which i it breaks on so that the issue is easily replicated.

Code snippet
```py
import numpy as np
from sklearn.linear_model import Lars
import sklearn

flag = True
print sklearn.__version__
s = 100
while flag:
    np.random.seed(s)
    X = np.random.randn(10,5)
    Y = np.random.randn(10)
    l=Lars(fit_intercept=False, positive=True)

    l.fit(X,Y)
    print l.coef_
    for i in l.coef_ :
        if i < 0 :
            flag = False
    s += 1
```

Output
```
0.19.0
[ 0.18760379  0.25412609  0.          0.          0.        ]
[ 0.          0.07212829  0.19939359  0.          0.0600625 ]
[ 0.          0.09812548  0.30880926  0.          0.21128258]
[ 0.          0.          0.          0.          0.25207725]
[ 0.21952703  0.07202002  0.44614742  0.          0.        ]
[ 0.03364774  0.          0.04648766  0.          0.        ]
[  2.66874987e-04   7.74587735e-01   0.00000000e+00   5.53611627e-01
   8.67075723e-01]
[ 0.          0.22650594  0.05970705  0.          0.24344585]
[ 0.06457963  0.          0.          0.03992725  0.        ]
[ 0.         -0.02069482  0.          0.47615768  1.25238536]
```
And what is s at the end?
@jnothman random seed incremented at every iteration
@jnothman Sorry didn't  understand your question earlier. s=109 at the breaking point

Code snippet

```
import numpy as np
from sklearn.linear_model import Lars
import sklearn

flag = True
print sklearn.__version__
s = 100
while flag:
    np.random.seed(s)
    X = np.random.randn(10,5)
    Y = np.random.randn(10)
    l=Lars(fit_intercept=False, positive=True)

    l.fit(X,Y)
    print l.coef_
    for i in l.coef_ :
        if i < 0 :
            print s
            flag = False
    s += 1

```
Output 
```
0.19.0
[ 0.18760379  0.25412609  0.          0.          0.        ]
[ 0.          0.07212829  0.19939359  0.          0.0600625 ]
[ 0.          0.09812548  0.30880926  0.          0.21128258]
[ 0.          0.          0.          0.          0.25207725]
[ 0.21952703  0.07202002  0.44614742  0.          0.        ]
[ 0.03364774  0.          0.04648766  0.          0.        ]
[  2.66874987e-04   7.74587735e-01   0.00000000e+00   5.53611627e-01
   8.67075723e-01]
[ 0.          0.22650594  0.05970705  0.          0.24344585]
[ 0.06457963  0.          0.          0.03992725  0.        ]
[ 0.         -0.02069482  0.          0.47615768  1.25238536]
109

```
So the minimal reproducing example is:

```python
import numpy as np
from sklearn.linear_model import Lars
np.random.seed(109)
X = np.random.randn(10,5)
Y = np.random.randn(10)
l = Lars(fit_intercept=False, positive=True)
l.fit(X,Y)
print(l.coef_)
assert(np.all(l.coef_ >= 0))
```

Thanks. I can reproduce, indeed.
And the bug appears to have been present since `positive` was introduced.
Hi @jnothman, Can I work on this issue?
If you're confident you can debug it, you're more than welcome to!

Commit message: more docstring updates

Patch to review:
diff --git a/doc/whats_new/v0.20.rst b/doc/whats_new/v0.20.rst
index afd3ed299585b..6cdfcb9196c3e 100644
--- a/doc/whats_new/v0.20.rst
+++ b/doc/whats_new/v0.20.rst
@@ -242,6 +242,10 @@ Linear, kernelized and related models
   underlying implementation is not random.
   :issue:`9497` by :user:`Albert Thomas <albertcthomas>`.
 
+- Deprecate ``positive=True`` option in :class:`linear_model.Lars` as the
+  underlying implementation is broken. Use :class:`linear_model.Lasso` instead.
+  :issue:`9837` by `Alexandre Gramfort`_.
+
 Metrics
 
 - Deprecate ``reorder`` parameter in :func:`metrics.auc` as it's no longer required
diff --git a/sklearn/linear_model/least_angle.py b/sklearn/linear_model/least_angle.py
index 88fae8aa72934..5ce0a38edee6b 100644
--- a/sklearn/linear_model/least_angle.py
+++ b/sklearn/linear_model/least_angle.py
@@ -98,11 +98,10 @@ def lars_path(X, y, Xy=None, Gram=None, max_iter=500,
 
     positive : boolean (default=False)
         Restrict coefficients to be >= 0.
-        When using this option together with method 'lasso' the model
+        This option is only allowed with method 'lasso'. Note that the model
         coefficients will not converge to the ordinary-least-squares solution
-        for small values of alpha (neither will they when using method 'lar'
-        ..). Only coefficients up to the smallest alpha value
-        (``alphas_[alphas_ > 0.].min()`` when fit_path=True) reached by the
+        for small values of alpha. Only coefficients up to the smallest alpha
+        value (``alphas_[alphas_ > 0.].min()`` when fit_path=True) reached by the
         stepwise Lars-Lasso algorithm are typically in congruence with the
         solution of the coordinate descent lasso_path function.
 
@@ -145,6 +144,11 @@ def lars_path(X, y, Xy=None, Gram=None, max_iter=500,
            <https://en.wikipedia.org/wiki/Lasso_(statistics)>`_
 
     """
+    if method == 'lar' and positive:
+        warnings.warn('positive option is broken for Least '
+                      ' Angle Regression (LAR). Use method="lasso".'
+                      ' This option will be removed in version 0.21.',
+                      DeprecationWarning)
 
     n_features = X.shape[1]
     n_samples = y.size
@@ -545,6 +549,8 @@ class Lars(LinearModel, RegressorMixin):
         Restrict coefficients to be >= 0. Be aware that you might want to
         remove fit_intercept which is set True by default.
 
+        The option is deprecated and will be removed in v0.21.
+
     Attributes
     ----------
     alphas_ : array, shape (n_alphas + 1,) | list of n_targets such arrays
@@ -1033,6 +1039,7 @@ class LarsCV(Lars):
         Restrict coefficients to be >= 0. Be aware that you might want to
         remove fit_intercept which is set True by default.
 
+        The option is deprecated and will be removed in v0.21.
 
     Attributes
     ----------
diff --git a/sklearn/linear_model/tests/test_least_angle.py b/sklearn/linear_model/tests/test_least_angle.py
index 0586b8433943d..e41df9cce1178 100644
--- a/sklearn/linear_model/tests/test_least_angle.py
+++ b/sklearn/linear_model/tests/test_least_angle.py
@@ -478,27 +478,37 @@ def test_lars_path_positive_constraint():
     # ensure that we get negative coefficients when positive=False
     # and all positive when positive=True
     # for method 'lar' (default) and lasso
-    for method in ['lar', 'lasso']:
-        alpha, active, coefs = \
-            linear_model.lars_path(diabetes['data'], diabetes['target'],
-                                   return_path=True, method=method,
-                                   positive=False)
-        assert_true(coefs.min() < 0)
 
-        alpha, active, coefs = \
-            linear_model.lars_path(diabetes['data'], diabetes['target'],
-                                   return_path=True, method=method,
-                                   positive=True)
-        assert_true(coefs.min() >= 0)
+    # Once deprecation of LAR + positive option is done use these:
+    # assert_raises(ValueError, linear_model.lars_path, diabetes['data'],
+    #               diabetes['target'], method='lar', positive=True)
+
+    with warnings.catch_warnings(record=True) as w:
+        linear_model.lars_path(diabetes['data'], diabetes['target'],
+                               return_path=True, method='lar',
+                               positive=True)
+    assert_true(len(w) == 1)
+    assert "broken" in str(w[0].message)
+
+    method = 'lasso'
+    alpha, active, coefs = \
+        linear_model.lars_path(diabetes['data'], diabetes['target'],
+                               return_path=True, method=method,
+                               positive=False)
+    assert_true(coefs.min() < 0)
+
+    alpha, active, coefs = \
+        linear_model.lars_path(diabetes['data'], diabetes['target'],
+                               return_path=True, method=method,
+                               positive=True)
+    assert_true(coefs.min() >= 0)
 
 
 # now we gonna test the positive option for all estimator classes
 
 default_parameter = {'fit_intercept': False}
 
-estimator_parameter_map = {'Lars': {'n_nonzero_coefs': 5},
-                           'LassoLars': {'alpha': 0.1},
-                           'LarsCV': {},
+estimator_parameter_map = {'LassoLars': {'alpha': 0.1},
                            'LassoLarsCV': {},
                            'LassoLarsIC': {}}
 


Code review comments:
diff_hunk: @@ -1033,6 +1039,7 @@ class LarsCV(Lars):
         Restrict coefficients to be >= 0. Be aware that you might want to
         remove fit_intercept which is set True by default.
 
+        The option is deprecated and will be removed in v0.21.
path: sklearn/linear_model/least_angle.py
text: @user1:
mention that it's broken

diff_hunk: @@ -545,6 +549,8 @@ class Lars(LinearModel, RegressorMixin):
         Restrict coefficients to be >= 0. Be aware that you might want to
         remove fit_intercept which is set True by default.
 
+        The option is deprecated and will be removed in v0.21.
path: sklearn/linear_model/least_angle.py
text: @user1:
mention that it's broken.