Skip to content

Add num_threads in the prange loop of init_bounds in KMeans #22773

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 14, 2022
Merged

Add num_threads in the prange loop of init_bounds in KMeans #22773

merged 1 commit into from
Mar 14, 2022

Conversation

jeremiedbb
Copy link
Member

Fixes #20483

@jeremiedbb jeremiedbb added the Quick Review For PRs that are quick to review label Mar 11, 2022
@adrinjalali
Copy link
Member

Can this be tested? 😁

@jeremiedbb
Copy link
Member Author

I don't see how to do that easily. We don't do it for the other pranges. It should mainly be noticeable when running in e.g a docker container with quota on cpu ressources on a machine with many cores.

@thomasjpfan
Copy link
Member

At a high level, we already test that the results are the same with different n_threads setting:

scikit-learn/sklearn/cluster/tests/test_k_means.py

Lines 932 to 934 in f9d7423

def test_result_equal_in_diff_n_threads(Estimator):
# Check that KMeans/MiniBatchKMeans give the same results in parallel mode
# than in sequential mode.

@jeremiedbb
Copy link
Member Author

jeremiedbb commented Mar 12, 2022 •
edited
Loading

This PR is not about parallelizing a some part of the code. This function is already parallel, it's just that we don't control the number of threads the way we do for other pranges, i.e. with _openmp_effective_n_threads. That's why testing this change is particularily hard. The parallelism in itself is indeed already covered by the test mentioned above.

thomasjpfan
thomasjpfan approved these changes Mar 12, 2022
View reviewed changes
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

@thomasjpfan
Copy link
Member

thomasjpfan commented Mar 12, 2022 •
edited
Loading

Sorry for being unclear. I was trying to say that we already test for different n_threads settings showing that this PR does not introduce any regressions.

As long as _openmp_effective_n_threads is correct, then I do not think we need to test for functions that use n_threads in terms of oversubscription.

ogrisel
ogrisel approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@ogrisel ogrisel 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. LGTM!

@ogrisel ogrisel merged commit 09a7293 into scikit-learn:main Mar 14, 2022
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Apr 6, 2022
@jeremiedbb @glemaitre
add num_threads in kmeans init_bounds (scikit-learn#22773)
511d232
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ogrisel ogrisel ogrisel approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
cython module:cluster Quick Review For PRs that are quick to review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Use _openmp_effective_n_threads to take cgroups CPU quotas into account into account in Elkan and Minibatch KMeans
4 participants
@jeremiedbb @adrinjalali @thomasjpfan @ogrisel