[MRG+1] Removing repeated input checking in Lasso and DictLearning #5133

arthurmensch · 2015-08-18T11:34:45Z

I use :

a check_input flag in coordinate_descent.enet_path, that is set to False when called from ElasticNet.fit
- a check_input flag in coordinate_descent.ElasticNet.fit, that is set to False when called from sparse_encode

I changed the condition for overriding provided Gram Matrix in linear_model.base._pre_fit, in order not to do a pass on data when fit_intercept and normalize are set to False (in master, we override Gram matrix if we find that X was changed by centering and rescaling, even when these are disabled, which wastes computation).

I also avoid computing cov in sparse_encode when using coordiante_descent, as cov computation is done within the Lasso class.

On provided plot_online_sparse_pca example, we gain a factor 2 in performance.

agramfort · 2015-08-18T12:31:23Z

sklearn/linear_model/base.py

@@ -421,12 +426,12 @@ def _pre_fit(X, y, Xy, precompute, normalize, fit_intercept, copy):
        precompute = (n_samples > n_features)

    if precompute is True:
-        precompute = np.dot(X.T, X)
+        precompute = np.dot(X.T, X).T


same rmk here

agramfort · 2015-08-18T12:33:00Z

do we really want to add this example?

travis is not happy.

amueller · 2015-08-18T15:00:02Z

examples/decomposition/plot_online_sparse_pca.py

@@ -0,0 +1,104 @@
+"""


Don't we already have this here: http://scikit-learn.org/dev/auto_examples/decomposition/plot_faces_decomposition.html ?

arthurmensch · 2015-08-18T15:15:31Z

This example has no point I only use it for benchmark. I am putting into a seperate gist

arthurmensch · 2015-08-18T16:59:28Z

I added a pool context manager in dict_learning.py (should probably put into another PR, although this one actually aims at allowing multiprocessing). Here is what I find :

In all cases, it is faster to use the context manager (that was expected)
Working on small X / small y (i.e small batches), it is faster to use multiprocessing backend, due to the fact that Lasso.fit has only a small part (~20%) of its computation performed with GIL released. Using threading causes too much congestion, with smaller pool management (1 ms for n_jobs = 2) overhead than multiprocessing (20 ms for n_jobs = 1). In this regime however, using n_jobs > 1 results at best in the same computation time as n_jobs = 1. In this case, I think we should not allow multiprocessing.
Working on larger X, y, it is faster to use threading backend, due to the fact that the cython part has more importance (~60%). Overhead is the same as above, but becomes negligible.

On a side note, I encountered problem with the context manger in multiprocessing mode with large data (see joblib/joblib#229)

We obtain small improvement working on samples of dimension 409 600 and batch_size = 400, as sparse_encode takes 25% less time. However, in this regime, _update_dict and auxiliary variable update becomes more important, which mitigates the improvement : around 10 % using 2 cores, and not significant imrpovement with more cores.

We should thus allow multiprocessing only past a certain threshold of batch_size * n_features in dictionary learning, if we want to allow it at all. I need precise benchmarking to set this threshold.

ogrisel · 2015-08-19T08:01:22Z

sklearn/linear_model/base.py

@@ -397,7 +398,8 @@ def fit(self, X, y):
        return self


-def _pre_fit(X, y, Xy, precompute, normalize, fit_intercept, copy):
+def _pre_fit(X, y, Xy, precompute, normalize, fit_intercept, copy,
+             aux_order=None):


I don't understand what "aux" stands for. Is it an acronym? A more explicit is probably required.

Basically this flag ask for Fortran order Xy and precompute. auxiliary_matrix_order ?

ogrisel · 2015-08-19T08:08:10Z

In order to move forward, I think this we should focus on fixing this PR (address the remaining comments and make the tests pass) and move the introduction a managed parallel context to a later PR.

ogrisel · 2015-08-19T08:10:24Z

sklearn/linear_model/coordinate_descent.py

@@ -585,7 +599,7 @@ class ElasticNet(LinearModel, RegressorMixin):
    def __init__(self, alpha=1.0, l1_ratio=0.5, fit_intercept=True,
                 normalize=False, precompute=False, max_iter=1000,
                 copy_X=True, tol=1e-4, warm_start=False, positive=False,
-                 random_state=None, selection='cyclic'):
+                 random_state=None, selection='cyclic', check_input=True):


Other scikit-learn models use check_input only as per-method parameters rather than global constructor parameter when used in estimators. I think we should stay consistent with that pattern.

arthurmensch · 2015-08-19T08:14:16Z

OK I will move multiprocessing to a later PR. For the record, here are the benchmarks I did with multiprocessing.

Without logscale

For very large sample we begin to see a small improvement (especially with 5 cores). Far from being outstanding though

arthurmensch · 2015-08-19T09:16:31Z

Comparison with master for 1 core, bypassing checks : green is master, blue is this PR.

Average improvement for a single iteration : x 3,47.

Time before the first iteration is a little longer as we perform initial input checking within dictionary_learning_online, but this is negligible compared to single iteration improvement.

arthurmensch · 2015-08-19T11:40:09Z

I will wait for reviews before opening the multiprocessing PR. Travis does not complain anymore.

I have not look into lars code (which is way slower for dictionary learning in scikit-learn, especially with this improvement), but it might be worth it to do the same changes.

ogrisel · 2015-08-19T12:02:32Z

sklearn/linear_model/tests/test_coordinate_descent.py

+    clf = ElasticNet(selection='cyclic', tol=1e-8)
+    # Check that no error is raised if data is provided in the right format
+    clf.fit(X, y, check_input=False)
+    X = check_array(X, order='F', dtype='float32')


I guess that if you try with the correct dtype but the wrong memory layout:

X = check_array(X, order='C', dtype='float64')

you should not get an error but a wrong results. This is expected but maybe this should be asserted in the tests as well.

ogrisel · 2015-08-19T12:06:01Z

This comment has not been addressed: https://github.com/scikit-learn/scikit-learn/pull/5133/files#r37388137

ogrisel · 2015-08-19T12:11:55Z

sklearn/decomposition/dict_learning.py

-        clf = Lasso(alpha=alpha, fit_intercept=False, precompute=gram,
-                    max_iter=max_iter, warm_start=True)
+        clf = Lasso(alpha=alpha, fit_intercept=False, normalize=False,
+                    precompute=gram, max_iter=max_iter, warm_start=True)


This makes me realise that we always use Lasso with precomputed Gram matrix in the context of online dictionary learning. Have you checked if this is actually a always a good idea from a performance point of view?

If instead we want to use allow the use of coordinate descent with row slices (sample-wise minibatch) of a Fortran aligned feature array then we would need to change the cython code of coordinate descent to deal properly with strided arrays (views) in the daxpy calls if we want to leverage the check_input=False code path safely.

This does not seem to be hard but maybe this should be investigated in a separate PR.

ogrisel · 2015-08-19T12:43:28Z

This looks good. Please add an entry in doc/whats_new.rst and mention the 3x speedup you measured.

ogrisel · 2015-08-19T12:45:20Z

Also please squash this PR into a single commit.

agramfort · 2015-08-19T13:26:23Z

sklearn/linear_model/base.py

@@ -77,7 +78,7 @@ def sparse_center_data(X, y, fit_intercept, normalize=False):
    return X, y, X_mean, y_mean, X_std


-def center_data(X, y, fit_intercept, normalize=False, copy=True,
+def  center_data(X, y, fit_intercept, normalize=False, copy=True,


oups. Extra space

agramfort · 2015-08-19T13:27:40Z

sklearn/linear_model/tests/test_coordinate_descent.py

+    coef_false = clf.coef_
+    clf.fit(X, y, check_input=True)
+    coef_true = clf.coef_
+    assert_true(np.any(coef_true != coef_false))


use assert_array_almost_equal

agramfort · 2015-08-19T13:28:24Z

besides LGTM if travis is happy.

arthurmensch · 2015-08-20T09:37:14Z

Done.

[MRG+1] Removing repeated input checking in Lasso and DictLearning

ogrisel · 2015-08-20T14:13:12Z

Great, thank for the optim @arthurmensch!

agramfort reviewed Aug 18, 2015
View reviewed changes

amueller reviewed Aug 18, 2015
View reviewed changes

arthurmensch changed the title ~~Removing repeated input checking in Lasso and DictLearning~~ [WIP] Removing repeated input checking in Lasso and DictLearning Aug 18, 2015

ogrisel reviewed Aug 19, 2015
View reviewed changes

arthurmensch force-pushed the lasso_perf_checks branch from 2dd6b5a to 9b2c894 Compare August 19, 2015 11:36

arthurmensch changed the title ~~[WIP] Removing repeated input checking in Lasso and DictLearning~~ [MRG] Removing repeated input checking in Lasso and DictLearning Aug 19, 2015

ogrisel reviewed Aug 19, 2015
View reviewed changes

ogrisel changed the title ~~[MRG] Removing repeated input checking in Lasso and DictLearning~~ [MRG+1] Removing repeated input checking in Lasso and DictLearning Aug 19, 2015

arthurmensch force-pushed the lasso_perf_checks branch from 3debe2b to 77b7012 Compare August 19, 2015 12:58

agramfort reviewed Aug 19, 2015
View reviewed changes

Added check_input in Lasso for DictLearning performance

2ed0c22

arthurmensch force-pushed the lasso_perf_checks branch from 9575b31 to 2ed0c22 Compare August 20, 2015 09:36

A93C
ogrisel added a commit that referenced this pull request Aug 20, 2015

Merge pull request #5133 from arthurmensch/lasso_perf_checks

393d651

[MRG+1] Removing repeated input checking in Lasso and DictLearning

ogrisel merged commit 393d651 into scikit-learn:master Aug 20, 2015

agramfort mentioned this pull request Jan 31, 2018

ElasticNet overwrites X even with copy_X=True #10540

Closed

jakirkham mentioned this pull request Jul 11, 2018

[MRG] MNT: Optionally skip another input check in ElasticNet, Lasso #11487

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Removing repeated input checking in Lasso and DictLearning #5133

[MRG+1] Removing repeated input checking in Lasso and DictLearning #5133

[MRG+1] Removing repeated input checking in Lasso and DictLearning #5133

[MRG+1] Removing repeated input checking in Lasso and DictLearning #5133

Conversation

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