Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 21, 2020

Conversation

alexhenrie
Copy link
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

Copy link
Member

@rth rth left a comment

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
Copy link
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
Copy link
Member

rth commented Jun 19, 2020

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
Copy link
Member

rth commented Jun 19, 2020

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.

Copy link
Member

@jnothman jnothman left a comment

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
Copy link
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
7 checks passed
@jnothman
Copy link
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
rubywerman pushed a commit to MLH-Fellowship/scikit-learn that referenced this pull request Jun 24, 2020
viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants