[MRG+1] remove n_nonzero_coefs from attr of LassoLarsCV + clean up call hierarchy by agramfort · Pull Request #9004 · scikit-learn/scikit-learn

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

                                 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

        # 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

+    # XXX deprecate?
    @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

        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

            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

Uh oh!

Conversation

agramfort commented Jun 6, 2017

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jun 6, 2017

Codecov Report

Uh oh!

agramfort commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vene commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaelVaroquaux commented Jun 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants