Extend sum reduction kernel, add argmin reduction kernel #46

fcharras · 2022-10-27T15:24:38Z

Two things in this PR:

extend the sum reduction kernel for 1d arrays to 2d arrays when reducing over axis 1
add a similar argmin kernel

Not sure what is better, using those kernels or using dpnp functions.

Pros for using those kernels:

limit the dependency to dpnp (which can especially be a good things if numba_dpex get interoperability to nvidia/amd gpus before dpnp does)
more confidence regarding the strategy for dispatching to thread that fit more GPU devices, it's not clear at what point dpnp can run well on gpus
fun

Cons for using those kernels:

it's verbose, and if we want to optimize other usecases (depending if axis0 is "big enough", or else which is bigger from axis 0 or axis 1,...) it would require yet other kernels.
it does not sound like it's a good thing to jit those simple operations, the added jit time might get annoying
at some point, it can be assumed that a reliable tensor library (probably dpnp ?) will be available and the kernels will be obsolete anyway.
even if the implementation fits the gpu model, I think the performance is not that great, it might become competitive if there's a public api in numba_dpex to use async dispatching like described in On making task configuration and task args available to the user without executing IntelPython/numba-dpex#769 .
in fact, the performance does not even really matter because until now I don't think it contributes much to total execution time anyway.

Those pro / cons can also be weighted for all other kernels in this file, implementations for most of those are already available in dpnp.

…d argmin reduction kernel

jjerphan

Thanks @fcharras. Here are a few comments.

sklearn_numba_dpex/common/kernels.py

fcharras · 2022-11-10T14:23:32Z

TODO: add couple of tests and merge

jjerphan

As discussed on a call with @fcharras, this is ready to merge modulo a few tests on the new common kernels.

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

fcharras · 2022-11-14T08:24:16Z

Tests added. LGTM ?

ogrisel

Here is my review.

sklearn_numba_dpex/common/tests/test_kernels.py

sklearn_numba_dpex/kmeans/drivers.py

sklearn_numba_dpex/common/kernels.py

ogrisel · 2022-11-14T09:05:56Z

sklearn_numba_dpex/common/kernels.py

        kernels_and_empty_tensors_pairs.append((kernel, result))

    def sum_reduction(summands):
+        # TODO: manually dispatch the kernels with a SyclQueue
        for kernel, result in kernels_and_empty_tensors_pairs:
            kernel(summands, result)
            summands = result


It would be more efficient to do the calls to dpt.empty(result_shape, dtype=dtype, device=device) lazily inside this loop, instead of allocating all the results buffers ahead of time?

Or maybe the Python GC would add sequential overhead that would kill the performance?

By allocating ahead of time we also keep the buffers allocated and re-use them across all future calls to this instance of sum_reduction. The buffers will be only be garbage collected when the instance of sum_reduction is garbage collected. A given instance of sum_reduction in our loops is going to be called once per iteration so I think it's more sensible this way.

Maybe add an inline comment to explain this!

sklearn_numba_dpex/common/kernels.py

sklearn_numba_dpex/common/tests/test_kernels.py

jjerphan

LGTM, @ogrisel outlined all everything already.

I guess, make_sum_reduction_2d_axis0_kernel can be implemented when need and those test generalised if it is the case. What do you think?

jjerphan · 2022-11-14T09:26:15Z

sklearn_numba_dpex/common/tests/test_kernels.py

+
+@pytest.mark.parametrize("dtype", float_dtype_params)
+def test_argmin_reduction_1d(dtype):
+    n_items = 4


Would it make sense to define n_items based on the length of array_in?

This comment also applies in other tests.

fcharras · 2022-11-14T09:30:34Z

@jjerphan

make_sum_reduction_2d_axis0_kernel can be implemented when need and those test generalised if it is the case.

what do you mean ? all the aspects in this PR are required for kmeans++

(edit: indeed _axis0 is not required only axis1)

jjerphan · 2022-11-14T09:39:51Z

I meant that make_sum_reduction_2d_axis1_kernel is implemented as it is needed for kmeans++, but make_sum_reduction_2d_axis0_kernel is not at the moment but might be needed in the future.

If make_sum_reduction_2d_axis0_kernel is implemented in the future, tests could be adapted accordingly: to me, nothing is to change for this PR.

ogrisel · 2022-11-14T14:24:05Z

Let's not worry about make_sum_reduction_2d_axis0_kernel before we ever need it and refactor only then if needed.

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

ogrisel

LGTM!

fcharras · 2022-11-15T07:42:52Z

Merged! ty for review

…d argmin reduction kernel (#46) Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* Add a module for rng kernels and kernel funcs * Test the RNG kernels against a reference implementation (randomgen) * Ensure that the float32 rng is equivalent to the float64 rng casted to float32 * Mimic https\:\/\/prng\.di\.unimi\.it\/xoroshiro128plusplus\.c implementation rather than `randomgen` and document the issue * Extend sum reduction kernel to axis1 reduction on 2d arrays and add 1d argmin reduction kernel * Working k-means++ * Port kmeansplusplus tests from sklearn test_k_means module * Reactivate sklearn relocation cluster unit test * Clarity and commenting Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> * Fix variable name * Overall cleaning kmeansplusplus Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> * test fix centers.dtype attr error * test fix centers.dtype attr error * test float32 and float64 rng, assert same rng, and commenting * Overall commenting and nits Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> * Clarity in tests for rng Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Extend sum reduction kernel, add argmin reduction kernel (#46) * Extend sum reduction kernel to axis1 reduction on 2d arrays and add 1d argmin reduction kernel Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Commenting + add a test for rng quality. Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Apply comment suggestions Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * fix: subsequence_start * centroid -> candidate * Apply docstring suggestions Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> * Apply comment suggestions & minor fixes * minor fix * Add a quality test for kmeans plusplus * Comment highlighting the equivalence of the results with engine and vanilla kmeans++ when evaluated with more iterations * Fix kmeans plusplus test * Enable k-means++ support of daal4py Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

fcharras requested review from jjerphan and ogrisel October 27, 2022 15:24

fcharras force-pushed the extend_sum_kernel_to_dim2 branch from 01e12bc to 48190dd Compare October 27, 2022 16:53

fcharras force-pushed the rng_kernels branch from 9a23db2 to 60e3ddc Compare October 27, 2022 16:55

fcharras force-pushed the extend_sum_kernel_to_dim2 branch 2 times, most recently from ef3975f to e2ebdfe Compare October 27, 2022 17:13

Extend sum reduction kernel to axis1 reduction on 2d arrays and add 1…

ee0897f

…d argmin reduction kernel

fcharras force-pushed the extend_sum_kernel_to_dim2 branch from e2ebdfe to ee0897f Compare October 31, 2022 08:46

fcharras mentioned this pull request Oct 31, 2022

FEA Implement kmeans++ with numba_dpex kernels #37

Merged

jjerphan reviewed Nov 3, 2022

View reviewed changes

Commenting naming and formating

b270697

jjerphan approved these changes Nov 10, 2022

View reviewed changes

Add tests for reduction kernels

6bed777

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>

fcharras requested a review from jjerphan November 14, 2022 08:57

ogrisel reviewed Nov 14, 2022

View reviewed changes

jjerphan approved these changes Nov 14, 2022

View reviewed changes

fcharras and others added 2 commits November 14, 2022 15:40

Comments and formating nits

6512a05

Co-authored-by: Julien Jerphanion <git@jjerphan.xyz> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

more tests and commenting

4c80039

fcharras requested a review from ogrisel November 14, 2022 17:33

ogrisel approved these changes Nov 14, 2022

View reviewed changes

fcharras merged commit 5235fb3 into rng_kernels Nov 15, 2022

fcharras deleted the extend_sum_kernel_to_dim2 branch November 15, 2022 07:31

fcharras mentioned this pull request Nov 15, 2022

Possible typo in kernels.py:sum_reduction function #54

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend sum reduction kernel, add argmin reduction kernel #46

Extend sum reduction kernel, add argmin reduction kernel #46

fcharras commented Oct 27, 2022 •

edited

Loading

jjerphan left a comment

fcharras commented Nov 10, 2022

jjerphan left a comment

fcharras commented Nov 14, 2022

ogrisel left a comment

ogrisel Nov 14, 2022

fcharras Nov 14, 2022 •

edited

Loading

ogrisel Nov 14, 2022

ogrisel Nov 14, 2022

jjerphan left a comment

jjerphan Nov 14, 2022

fcharras commented Nov 14, 2022 •

edited

Loading

jjerphan commented Nov 14, 2022 •

edited

Loading

ogrisel commented Nov 14, 2022

ogrisel left a comment

fcharras commented Nov 15, 2022

Extend sum reduction kernel, add argmin reduction kernel #46

Extend sum reduction kernel, add argmin reduction kernel #46

Conversation

fcharras commented Oct 27, 2022 • edited Loading

jjerphan left a comment

Choose a reason for hiding this comment

fcharras commented Nov 10, 2022

jjerphan left a comment

Choose a reason for hiding this comment

fcharras commented Nov 14, 2022

ogrisel left a comment

Choose a reason for hiding this comment

ogrisel Nov 14, 2022

Choose a reason for hiding this comment

fcharras Nov 14, 2022 • edited Loading

Choose a reason for hiding this comment

ogrisel Nov 14, 2022

Choose a reason for hiding this comment

ogrisel Nov 14, 2022

Choose a reason for hiding this comment

jjerphan left a comment

Choose a reason for hiding this comment

jjerphan Nov 14, 2022

Choose a reason for hiding this comment

fcharras commented Nov 14, 2022 • edited Loading

jjerphan commented Nov 14, 2022 • edited Loading

ogrisel commented Nov 14, 2022

ogrisel left a comment

Choose a reason for hiding this comment

fcharras commented Nov 15, 2022

fcharras commented Oct 27, 2022 •

edited

Loading

fcharras Nov 14, 2022 •

edited

Loading

fcharras commented Nov 14, 2022 •

edited

Loading

jjerphan commented Nov 14, 2022 •

edited

Loading