[MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy #9004

agramfort · 2017-06-06T12:46:32Z

Reference Issue

Fixes #8475

What does this implement/fix? Explain your changes.

introduce a private _fit method in Lars to fit with explicit params without
having to hack the instance attributes.

It also fixes a call to Lars.fit that was ignoring the max_iter parameter.

Any other comments?

Nope

agramfort · 2017-06-06T12:48:13Z

sklearn/linear_model/least_angle.py

@@ -370,11 +370,11 @@ def lars_path(X, y, Xy=None, Gram=None, max_iter=500,
            corr_eq_dir = np.dot(Gram[:n_active, n_active:].T,
                                 least_squares)

-        g1 = arrayfuncs.min_pos((C - Cov) / (AA - corr_eq_dir + tiny))
+        g1 = arrayfuncs.min_pos((C - Cov) / (AA - corr_eq_dir + tiny32))


tiny was not enough for #8475

and I think that 1.1754944e-38 is small enough (rather than 2.2250738585072014e-308)

agramfort · 2017-06-06T12:48:50Z

sklearn/linear_model/least_angle.py

-            alpha = 0.  # n_nonzero_coefs parametrization takes priority
-            max_iter = self.n_nonzero_coefs
-        else:
-            max_iter = self.max_iter


this has been moved to Lars.fit to avoid the magic of hacking instance attributes

agramfort · 2017-06-06T12:49:39Z

sklearn/linear_model/least_angle.py

@@ -1145,10 +1154,14 @@ def fit(self, X, y):
        # Now compute the full model
        # it will call a lasso internally when self if LassoLarsCV
        # as self.method == 'lasso'
-        Lars.fit(self, X, y)
+        Lars._fit(self, X, y, max_iter=self.max_iter, alpha=best_alpha,


now self.max_iter is not ignored

Is it the case that previously the call to Lars.fit reverted to a call to lasso_lars with alpha_min=0, while now alpha_min=best_alpha?

great! so we save a bit of computation that we didn't need to do previously.

Should we maybe just say self._fit now?

agramfort · 2017-06-06T12:50:25Z

sklearn/linear_model/least_angle.py

    @property
+    @deprecated("Attribute alpha is deprecated in 0.18 and "
+                "will be removed in 0.20. See 'alpha_' instead")


this was a hack to be able to call Lars.fit but the CV classes should have an alpha_ but no alpha param

amueller · 2017-06-06T13:11:01Z

sklearn/linear_model/tests/test_least_angle.py

@@ -402,6 +403,7 @@ def test_lars_cv():
        lars_cv.fit(X, y)
        np.testing.assert_array_less(old_alpha, lars_cv.alpha_)
        old_alpha = lars_cv.alpha_
+    assert_false(hasattr(lars_cv, 'n_nonzero_coefs'))


maybe a test for the ignored max_iter?

agramfort · 2017-06-06T13:14:02Z

sklearn/linear_model/least_angle.py

@@ -195,7 +195,6 @@ def lars_path(X, y, Xy=None, Gram=None, max_iter=500,
            sys.stdout.write('.')
            sys.stdout.flush()

-    tiny = np.finfo(np.float).tiny  # to avoid division by 0 warning
    tiny32 = np.finfo(np.float32).tiny  # to avoid division by 0 warning


for the record tiny32 was introduced here 294d4b6

codecov · 2017-06-06T23:38:38Z

Codecov Report

Merging #9004 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #9004      +/-   ##
==========================================
+ Coverage   95.91%   95.92%   +<.01%     
==========================================
  Files         331      331              
  Lines       59851    59960     +109     
==========================================
+ Hits        57409    57516     +107     
- Misses       2442     2444       +2

Impacted Files	Coverage Δ
sklearn/linear_model/least_angle.py	`96.05% <100%> (+0.15%)`	⬆️
sklearn/linear_model/tests/test_least_angle.py	`100% <100%> (ø)`	⬆️
sklearn/feature_selection/from_model.py	`91.66% <0%> (-2.37%)`	⬇️
sklearn/decomposition/dict_learning.py	`93.23% <0%> (-0.23%)`	⬇️
sklearn/decomposition/truncated_svd.py	`97.91% <0%> (-0.09%)`	⬇️
sklearn/feature_extraction/text.py	`96.02% <0%> (-0.03%)`	⬇️
sklearn/decomposition/tests/test_dict_learning.py	`100% <0%> (ø)`	⬆️
sklearn/feature_selection/rfe.py	`97.45% <0%> (ø)`	⬆️
sklearn/feature_selection/tests/test_from_model.py	`100% <0%> (ø)`	⬆️
sklearn/tests/test_multioutput.py	`100% <0%> (ø)`	⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b230c...ea66a5b. Read the comment docs.

agramfort · 2017-06-08T07:38:08Z

good to go on my end

appveyor error is an appveyor outage so is unrelated

vene · 2017-06-08T08:17:08Z

sklearn/linear_model/least_angle.py

-        """
-        X, y = check_X_y(X, y, y_numeric=True, multi_output=True)
+    def _fit(self, X, y, max_iter, alpha, fit_path, Xy=None):
+        """Aux method to fit the model using X, y as training data"""


I know it's private, but I think it should still say "Auxiliary" :P

vene · 2017-06-08T08:32:17Z

Other than the two extremely minor comments this LGTM. The regression test indeed fails on master and passes here. Clear improvement in terms of the code.

… to Lars._fit

GaelVaroquaux · 2017-06-10T14:45:02Z

sklearn/linear_model/least_angle.py

+        self.copy_X = copy_X
+        self.positive = positive
+        # XXX : we don't use super(LarsCV, self).__init__
+        # to avoid setting n_nonzero_coefs


Technically it tells us that we have the wrong inheritance diagram. But... I don't bother.

GaelVaroquaux · 2017-06-10T14:51:33Z

LGTM. Merging

…ll hierarchy (scikit-learn#9004) * FIX : remove n_nonzero_coefs from attr of LassoLarsCV + clean up call to Lars._fit * cleanup * fix deprecation warning + clarify warning * add test * pep8 * adddress comments

agramfort changed the title ~~remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy~~ [WIP] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy Jun 6, 2017

agramfort commented Jun 6, 2017

View reviewed changes

agramfort mentioned this pull request Jun 6, 2017

LassoLarsCV chatty but not that helpful. #8475

Closed

amueller reviewed Jun 6, 2017

View reviewed changes

agramfort commented Jun 6, 2017

View reviewed changes

agramfort force-pushed the fix_lasso_lars_cv_attr branch from ea66a5b to fde74da Compare June 7, 2017 08:15

agramfort changed the title ~~[WIP] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy~~ [MRG] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy Jun 7, 2017

vene reviewed Jun 8, 2017

View reviewed changes

vene changed the title ~~[MRG] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy~~ [MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy Jun 8, 2017

agramfort added 6 commits June 8, 2017 10:36

FIX : remove n_nonzero_coefs from attr of LassoLarsCV + clean up call…

8209200

… to Lars._fit

cleanup

1c2f4da

fix deprecation warning + clarify warning

2da8e32

add test

4fd1f4a

pep8

6f2c2a0

adddress comments

70338a0

agramfort force-pushed the fix_lasso_lars_cv_attr branch from 002c95a to 70338a0 Compare June 8, 2017 08:40

GaelVaroquaux reviewed Jun 10, 2017

View reviewed changes

GaelVaroquaux merged commit 32b88d8 into scikit-learn:master Jun 10, 2017

jnothman mentioned this pull request Nov 14, 2018

2 test failures on Debian stable (stretch) amd64 #12548

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy #9004

[MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy #9004

agramfort commented Jun 6, 2017

agramfort Jun 6, 2017

agramfort Jun 6, 2017

agramfort Jun 6, 2017

vene Jun 6, 2017

agramfort Jun 6, 2017

vene Jun 7, 2017

vene Jun 8, 2017

agramfort Jun 6, 2017

amueller Jun 6, 2017

agramfort Jun 6, 2017

agramfort Jun 6, 2017

codecov bot commented Jun 6, 2017

agramfort commented Jun 8, 2017

vene Jun 8, 2017

vene commented Jun 8, 2017

GaelVaroquaux Jun 10, 2017

GaelVaroquaux commented Jun 10, 2017

[MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy #9004

[MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy #9004

Conversation

agramfort commented Jun 6, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 6, 2017

Codecov Report

agramfort commented Jun 8, 2017

Choose a reason for hiding this comment

vene commented Jun 8, 2017

Choose a reason for hiding this comment

GaelVaroquaux commented Jun 10, 2017