Skip to content

PERF Don't return anything from neural net activation functions - #17633

Merged
jnothman merged 1 commit into
scikit-learn:masterfrom
alexhenrie:inplace
Jun 21, 2020
Merged

PERF Don't return anything from neural net activation functions#17633
jnothman merged 1 commit into
scikit-learn:masterfrom
alexhenrie:inplace

Conversation

@alexhenrie

@alexhenrie alexhenrie commented Jun 18, 2020

Copy link
Copy Markdown
Contributor

This pull request is similar to pull request #17604: Returning a reference to X from the activation functions is unnecessary and creates the false impression that they make copies. The activiation functions should have the same interface as the derivative functions, which were already returning nothing. Furthermore, dropping the return values improves the performance of neural net training by about 3%.

Test program:

from sklearn.datasets import load_digits
from sklearn.neural_network import MLPClassifier
from neurtu import delayed, Benchmark

digits = load_digits(return_X_y=True)
X = digits[0][:,:10]
y = digits[0][:,11]

clf = MLPClassifier(solver='lbfgs', alpha=1e-5,
                    hidden_layer_sizes=(5, 2), random_state=1, max_iter=1000)

train = delayed(clf).fit(X, y)
print(Benchmark(wall_time=True, cpu_time=True, repeat=10)(train))

Before:

      wall_time  cpu_time                                                                                                     
mean   2.036008  2.028113
max    2.060196  2.031687
std    0.008858  0.002375

After:

      wall_time  cpu_time                                                                                                     
mean   1.976324  1.970528
max    1.982843  1.977816
std    0.002903  0.003813

rth
rth approved these changes Jun 18, 2020
View reviewed changes

@rth rth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @alexhenrie !

Have you tried visualizing profiling results e.g. with snakeviz or py-spy? That could give you more ideas what to optimize..

@alexhenrie

alexhenrie commented Jun 18, 2020

Copy link
Copy Markdown
Contributor Author

Thanks Roman. I haven't tried snakeviz or py-spy yet. I do know that I could make the neural nets a lot faster by porting the code to Cython and using C functions instead of numpy functions, but I'm not sure if that's a project I want to take on.

@rth

rth commented Jun 19, 2020

Copy link
Copy Markdown
Member

I do know that I could make the neural nets a lot faster by porting the code to Cython and using C functions instead of numpy functions,

I wouldn't be so sure about it. After skimming though the implementation I couldn't see places were rewriting in Cython would be massively different (after a superficial look). Profiling would probably be a good start though.

@rth

rth commented Jun 19, 2020

Copy link
Copy Markdown
Member

Also it could be worth trying to storing / computing weights and activation in 32bit optionally, even if LBFGS would then need to cast back and forth to 64bit.

jnothman
jnothman approved these changes Jun 21, 2020
View reviewed changes

@jnothman jnothman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an entry to the change log at doc/whats_new/v0.24.rst. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:

@alexhenrie

alexhenrie commented Jun 21, 2020

Copy link
Copy Markdown
Contributor Author

I've made a few nice improvements to the neural net performance, but I don't think they deserve a change log entry because they're all pretty small.

@jnothman
jnothman merged commit c9c1a81 into scikit-learn:master Jun 21, 2020
@jnothman

jnothman commented Jun 21, 2020

Copy link
Copy Markdown
Member

It's worth having an entry for all together listing the relevant pull requests. If nothing else, this can be helpful if one of your changes accidentally creates a bug.

@alexhenrie
alexhenrie deleted the inplace branch June 21, 2020 02:44
dsandeep0138 pushed a commit to dsandeep0138/scikit-learn that referenced this pull request Jun 21, 2020
@alexhenrie @dsandeep0138
PERF Don't return anything from neural net activation functions (scik…
db999f6 
…it-learn#17633)
rubywerman pushed a commit to MLH-Fellowship/scikit-learn that referenced this pull request Jun 24, 2020
@alexhenrie @rubywerman
PERF Don't return anything from neural net activation functions (scik…
1992d88 
…it-learn#17633)
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@alexhenrie @viclafargue
PERF Don't return anything from neural net activation functions (scik…
4b28c1f 
…it-learn#17633)
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@alexhenrie @jayzed82
PERF Don't return anything from neural net activation functions (scik…
e558e61 
…it-learn#17633)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes
+1 more reviewer
@rth rth rth approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

module:neural_network

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@alexhenrie @rth @jnothman