Skip to content

[MRG] Parallelisation of decomposition/sparse_encode #13005

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

Merged
merged 3 commits into from
Jan 20, 2019

Conversation

nixphix
Copy link
Contributor

@nixphix nixphix commented Jan 17, 2019

Reference Issues/PRs

xref #12955

What does this implement/fix? Explain your changes.

sparse encode (dict learning uses sparse encode) is running in single-threaded mode irrespective of n_jobs parameters, the parallel execution code is unreachable since effective_n_jobs always returns a positive integer.

if effective_n_jobs(n_jobs) or algorithm == 'threshold':
code = _sparse_encode(X,
dictionary, gram, cov=cov,
algorithm=algorithm,
regularization=regularization, copy_cov=copy_cov,
init=init,
max_iter=max_iter,
check_input=False,
verbose=verbose,
positive=positive)
return code
# Enter parallel code block
code = np.empty((n_samples, n_components))

Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

thanks, please add a what's new entry

@nixphix nixphix changed the title [MRG] Parallelisation of dict learning [MRG] Parallelisation of decomposition/sparse_encode Jan 18, 2019
@nixphix nixphix force-pushed the fix/dict-learning-n-jobs branch from f2ee8d9 to a3a3114 Compare January 18, 2019 16:21
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

thanks @nixphix

:mod:`sklearn.decomposition`
...........................

- |Fix| Fixed a bug in :meth:`decomposition.sparse_encode` where computation was single
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use :func: here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@qinhanmin2014 qinhanmin2014 added this to the 0.20.3 milestone Jan 20, 2019
@nixphix nixphix force-pushed the fix/dict-learning-n-jobs branch from a3a3114 to 32c422d Compare January 20, 2019 04:24
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.

Good catch! Have you looked for other instances from when we introduced effective_n_jobs?

@jnothman jnothman merged commit db17f3e into scikit-learn:master Jan 20, 2019
@qinhanmin2014
Copy link
Member

Good catch! Have you looked for other instances from when we introduced effective_n_jobs?

I've checked all the effective_n_jobs in the repo and didn't find any bugs.

thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 7, 2019
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Feb 19, 2019
@jnothman jnothman mentioned this pull request Feb 19, 2019
17 tasks
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
@kausiala
Copy link

kausiala commented Jun 3, 2019

I have a multiprocessing setup, where each process runs a SparseCoder with n_jobs=1, and in the main thread I split my data to these worker processes. In scikit-learn=0.20.2 it works perfectly fine (parallelization makes processing faster as expected), but when I updated to 0.20.3 it gets super slow (parallel computing takes ~10 times longer than doing it in just one process).

koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants