[MRG+2] Make CD use fused types #6913

yenchenlin · 2016-06-21T01:34:06Z

According to #5464, current implementation of ElasticNet and Lasso in scikit-learn constrain the input to be np.float64, which is a waste of space.

This PR try to make CD algorithms support fused types when fitting np.float32 dense data and therefore reduce redundant data copy.

Make inline helper functions support fused types
Make dense CD ElasticNet support fused types
Add warning when alpha close to zero and X is np.float32
Add tests

**UPDATE 7/7

Here is the memory profiling results when fitting np.float32 data:

master

this branch

**UPDATE 7/12

Here is the memory profiling results when fitting sparse np.float32 data:

master

this branch

agramfort · 2016-06-21T04:08:58Z

do you expect to merge just this or is it wip?

yenchenlin · 2016-06-21T04:33:44Z

@agramfort Yeah I am thinking of merging just these as a start point.

jnothman · 2016-06-21T12:29:32Z

I'm not sure what value there is in merging it separately to something we can benchmark. For instance, you've fused fmax and fsign, but reused the libc fabs which explicitly operates on a double (as opposed to fabsl). Are you sure we benefit from fused implementations of max and sign?

So I think we want to review this as a whole.

yenchenlin · 2016-06-23T02:19:22Z

Thanks @jnothman @agramfort .
Ah yeah you are right, I am working! 💪

yenchenlin · 2016-07-01T23:51:22Z

***Updated 7/7

~~Currently it is still not working.~~
It is now working!

Here is my test script:

import numpy as np
from sklearn.linear_model.coordinate_descent import ElasticNet
from sys import argv

@profile
def fit_est():
    clf.fit(X, y)

np.random.seed(5)
X = np.random.rand(2000000, 40)
X = np.float32(X)
y = np.random.rand(2000000)
y = np.float32(y)
T = np.random.rand(5, 40)
T = np.float32(T)

clf = ElasticNet(alpha=1e-7, l1_ratio=1.0, precompute=False)
fit_est()
pred = clf.predict(T)
print pred

jnothman · 2016-07-04T04:23:44Z

sklearn/linear_model/cd_fast.pyx

-
-                # np.dot(R.T, y)
-                gap += (alpha * l1_norm - const * ddot(
+        if floating is double:


Are you absolutely certain we can't do this with fused types? What prohibits it?

This algorithm uses lots of C pointer such as <DOUBLE*>, we can't do <floating*>.

You're saying Cython disallows fused type pointers, or typecasts? Is that incapability documented?

Could we use typecasts if we were working with typed memoryviews?

jnothman · 2016-07-04T11:17:01Z

Could you use the line-based memory profiling to see where that sharp increase in memory consumption is coming in?

jnothman · 2016-07-04T11:17:27Z

Sorry, that was silly; only appropriate if the bad memory usage is in Python code.

jnothman · 2016-07-05T00:31:50Z

sklearn/linear_model/base.py

-            fit_intercept and not np.allclose(X_offset, np.zeros(n_features)) or
-            normalize and not np.allclose(X_scale, np.ones(n_features))):
+            fit_intercept and not np.allclose(X_offset, np.zeros(n_features))
+            or normalize and not np.allclose(X_scale, np.ones(n_features))):


I don't see why this is an improvement, or why it's in this PR.

yenchenlin · 2016-07-07T15:44:04Z

Hello @jnothman & @MechCoder , thanks alot-alot-alot-alot for your patience and comments.

I've updated the PR description (including new memory profiling results), code, and addressed comments you gave before.

The remaining to-do tasks in my opinion are also listed in the main description of this PR.

yenchenlin · 2016-07-08T09:52:50Z

I've added the tests and the user warning for potential non-convergence error when fitting np.float32 data with small alpha.

However, the CI looks weird, any idea?

yenchenlin · 2016-08-24T12:10:04Z

@jnothman done!

jnothman · 2016-08-24T12:28:47Z

Have you forgotten to push those last changes? whats_new does not appear updated, nor the warning.

jnothman · 2016-08-24T12:40:26Z

sklearn/linear_model/coordinate_descent.py

@@ -474,7 +474,8 @@ def enet_path(X, y, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
            warnings.warn('Objective did not converge.' +
                          ' You might want' +
                          ' to increase the number of iterations.' +
-                          ' Fitting data with alpha near zero, e.g., 1e-8,' +
+                          ' Fitting float32 data with alpha near zero,' +


But this is only relevant if the data is float32, no?

Actually I think fitting with a really small alpha, e.g., 1e-20, even float64 data may not converge.

Sure, so make the warning as relevant and useful as possible to a user that triggers it.

So is simply remove the float32 enough 😛?

Not really, because alpha=1e-8 isn't ordinarily too small for normalized float64. Either remove reference to the alpha value or check appropriate conditions for the message, then it will be much more meaningful message. Also "alpha near zero" would usually be "very small alpha".

I have to admit that I'm not very sure about all the factors that will cause convergence issue, and thus not dare to determine a specific reference value.

Or we can remove reference to the alpha value?

Remove any specific value. Just say near zero

On 25 August 2016 at 23:34, Yen notifications@github.com wrote:

In sklearn/linear_model/coordinate_descent.py
#6913 (comment)
:

@@ -474,7 +474,8 @@ def enet_path(X, y, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
warnings.warn('Objective did not converge.' +
' You might want' +
' to increase the number of iterations.' +

' Fitting data with alpha near zero, e.g., 1e-8,' +

' Fitting float32 data with alpha near zero,' +

I have to admit that I'm not very sure about all the factors that will
cause convergence issue, and thus not dare to determine a specific
reference value.

Or we can remove reference to the alpha value?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/scikit-learn/scikit-learn/pull/6913/files/82fdf0962e3c7b0965b54ca137a56ab6d01fc226..c032d3b5820b53fb0717008435a44245cdb746f1#r76242986,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEz65p1hO8jA-O7s1Ka5BIexYN_p7qlks5qjZnKgaJpZM4I6SId
.

MechCoder · 2016-08-24T21:07:34Z

sklearn/linear_model/coordinate_descent.py

@@ -470,7 +473,10 @@ def enet_path(X, y, l1_ratio=0.5, eps=1e-3, n_alphas=100, alphas=None,
        if dual_gap_ > eps_:
            warnings.warn('Objective did not converge.' +
                          ' You might want' +
-                          ' to increase the number of iterations',
+                          ' to increase the number of iterations.' +
+                          ' Fitting data with alpha near zero,' +


Did we not agree to add this message only when alpha is less than some heuristic value?

It seems hard to determine a heuristic value 😭

There are too many factors.

MechCoder · 2016-08-24T21:10:35Z

Your benchmarks in the description of the Pull Request suggests non-trivial speed gains. Do the speed gains also still hold?

yenchenlin · 2016-08-25T13:30:12Z

@MechCoder yes!

float32
float64

MechCoder · 2016-08-25T16:32:39Z

Awesome! Merging with master and thanks a lot for your perseverance.

yenchenlin · 2016-08-25T16:37:36Z

😭😭 😭

🍻🍻🍻

MechCoder · 2016-08-25T16:39:29Z

It should be worth adding a note to src/cblas/README.txt to let know what changes have to be made to add to call cblas functions internally. Maybe @fabianp can do that?

jnothman · 2016-08-26T02:36:09Z

Hurrah! Thanks @fabianp for rescuing this. And to @MechCoder for inviting that saviour. And to @yenchenlin for winning.

MechCoder · 2016-08-26T02:41:15Z

And to you the dark knight? :p

jnothman · 2016-09-06T03:46:34Z

Just a heads up that I'm a little concerned that these changes to Lasso changed its behaviour (for float64 data). It seems to have resulted in a test failure at #6717 (comment). For the dummy data I've tried so far, behaviour isn't changed, so this needs more verification.

…learn#6913) ElasticNet and Lasso no longer implicitly convert float32 dtype input to float64 internally. * Make helper functions in cd use fused types * Import cblas float functions * Make enet_coordinate_descent support fused types * Make dense case work * Refactor format * Remove redundant change * Add cblas files * Avoid redundant code * Remove redundant c files and import * Recover unnecessary change * Update comment * Make coef_ type consistent * Test float32 input * Add user warning when fitting float32 data with small alpha * Fix bug * Change variable to floating type * Make cd sparse support fused types * Make CD support fused types when data is sparse * Add referenced src files * Avoid duplicated code * Avoid type casting * Fix indentation in test * Avoid type casting in sparse implementation * Fix indentation * Fix duplicated intialization code * Follow PEP8 * Raise tmp precision to double * Add 64 bit computer check * Fix test * Add constraint * PEP 8 * Make saxpy have the same structure as daxpy Hopefully this fixes the problems outlined in PR scikit-learn#6913 * Remove wrong hardware test * Remove dsdot * Remove redundant asarray * Add test for fit_intercept * Make _preprocess_data support other dtypes * Add concrete value * Workaround * Fix error msg * Move declarartion * Remove redundant comment * Add tests * Test normalize * Delete warning * Fix comment * Add error msg * Add error msg * Add what's new * Fix error msg

yenchenlin mentioned this pull request Jun 21, 2016

[WIP] Make inline and helper functions in coordinate descent to support fused types #6905

Closed

yenchenlin changed the title ~~Make helper functions in cd use fused types~~ [MRG] Make helper functions in cd use fused types Jun 21, 2016

yenchenlin changed the title ~~[MRG] Make helper functions in cd use fused types~~ [WIP] Make CD use fused types Jun 23, 2016

yenchenlin force-pushed the cd-fused-types branch 2 times, most recently from 0bab8d3 to bb6ec9b Compare July 1, 2016 23:49

yenchenlin force-pushed the cd-fused-types branch 4 times, most recently from 8f79269 to 61a7938 Compare July 2, 2016 14:10

jnothman reviewed Jul 4, 2016
View reviewed changes

jnothman reviewed Jul 5, 2016
View reviewed changes

yenchenlin force-pushed the cd-fused-types branch 2 times, most recently from 92e2aad to 82f6f9c Compare July 7, 2016 14:55

yenchenlin changed the title ~~[WIP] Make CD use fused types~~ [WIP] Make dense CD use fused types Jul 7, 2016

yenchenlin changed the title ~~[WIP] Make dense CD use fused types~~ [MRG] Make dense CD use fused types Jul 8, 2016

yenchenlin added 8 commits August 24, 2016 20:08

Fix error msg

116ec79

Move declarartion

470d8ab

Remove redundant comment

f868af7

Add tests

0e88af2

Test normalize

14237e8

Delete warning

8000

b4b9cf1

Fix comment

9348ad7

Add error msg

82fdf09

yenchenlin force-pushed the cd-fused-types branch from e784ecd to 132d9fb Compare August 24, 2016 12:09

yenchenlin force-pushed the cd-fused-types branch from 132d9fb to 82fdf09 Compare August 24, 2016 12:20

jnothman reviewed Aug 24, 2016
View reviewed changes

Add error msg

d0b56bb

yenchenlin force-pushed the cd-fused-types branch from c032d3b to d5e73ae Compare August 24, 2016 12:54

Add what's new

611b412

yenchenlin force-pushed the cd-fused-types branch from d5e73ae to 611b412 Compare August 24, 2016 12:55

MechCoder reviewed Aug 24, 2016
View reviewed changes

Fix error msg

00cadb6

MechCoder merged commit 084ef97 into scikit-learn:master Aug 25, 2016

tguillemot mentioned this pull request Dec 15, 2016

[MRG+3] Fused types for MultiTaskElasticNet #8061

Merged

Uh oh!

[MRG+2] Make CD use fused types #6913

[MRG+2] Make CD use fused types #6913

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!