Skip to content

Conversation

@jjerphan
Copy link
Owner

@jjerphan jjerphan commented Dec 1, 2021

Reference Issues/PRs

Small rework for scikit-learn#21462

What does this implement/fix? Explain your changes.

Addresses discussions in https://github.com/scikit-learn/scikit-learn/pull/21462/files#r744318407

cc @thomasjpfan

@jjerphan jjerphan marked this pull request as ready for review December 1, 2021 13:42
Co-authored-by: Olivier Grisel <[email protected]>
@jjerphan
Copy link
Owner Author

jjerphan commented Dec 2, 2021

WDYT, @thomasjpfan?

jjerphan and others added 6 commits December 16, 2021 16:17
This computes the exact number of threads needed (effective
threads) at the initialisation.

This allows allocating the exact number of pointers that we
need.

This also allows removing the `num_threads` parameters
from the callbacks methods' signatures (as the information
is now accessible via an attribute).

Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
jjerphan and others added 2 commits December 22, 2021 09:33
This was mentionned by one of Oliviers' review but I forgot it.

Co-authored-by: Olivier Grisel <[email protected]>
Copy link

@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.

I have a few more thoughts, but I think it's time to merge (I can not merge into your fork). It would be good to see how the original PR looks now.

@jjerphan
Copy link
Owner Author

Merging based on @thomasjpfan's last comment: #5 (review)

I'll update the plan with this sub-PR reviews' comments that I see there.

@jjerphan jjerphan merged commit 8a0771a into pairwise-distances-argkmin Dec 23, 2021
@jjerphan jjerphan deleted the pairwise-distances-argkmin-raii branch December 23, 2021 08:26
jjerphan pushed a commit that referenced this pull request Jun 20, 2022
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.

4 participants