ENH: kernels for `random.vonmisses`; part 2 #681

samir-nasibli · 2021-04-14T18:46:27Z

Description

Enable computations on devices [CPU/GPU].

Tests

DPNP own:

tests/test_random.py::TestDistributionsVonmises::test_moments[large_kappa] PASSED
tests/test_random.py::TestDistributionsVonmises::test_moments[small_kappa] PASSED
tests/test_random.py::TestDistributionsVonmises::test_invalid_args PASSED
tests/test_random.py::TestDistributionsVonmises::test_seed[large_kappa] FAILED
tests/test_random.py::TestDistributionsVonmises::test_seed[small_kappa] FAILED

+ numpy external

TODO

tests/test_random.py::TestDistributionsVonmises::test_seed failed on both devices. Bug.

shssf · 2021-05-13T15:38:16Z

dpnp/backend/kernels/dpnp_krnl_random.cpp

@@ -1242,65 +1243,70 @@ void dpnp_rng_vonmises_large_kappa_c(void* result, const _DataType mu, const _Da

    Uvec = reinterpret_cast<_DataType*>(dpnp_memory_alloc_c(size * sizeof(_DataType)));
    Vvec = reinterpret_cast<_DataType*>(dpnp_memory_alloc_c(size * sizeof(_DataType)));
-
-    for (size_t n = 0; n < size;)
+    n = reinterpret_cast<size_t*>(dpnp_memory_alloc_c(sizeof(size_t)));


this is quite strange (Make scalar as a array with one element).
I think it should be a scalar, not an array.

scalar and this is the same. You just can not pass n to sycl region in other way.

shssf · 2021-05-13T15:40:12Z

dpnp/backend/kernels/dpnp_krnl_random.cpp

+                        Y = 0.0;
+                    else if (Y > 1.0)
+                        Y = 1.0;
+                    n[0] = n[0] + 1;


This is a mistake. This is parallel environment (SYCL kernel). Writing inside the kernel into same memory cause https://en.wikipedia.org/wiki/Race_condition

shssf · 2021-05-13T15:44:54Z

dpnp/backend/kernels/dpnp_krnl_random.cpp

-            V = Vvec[i];
-            sn2 = sn * sn;
-            cn2 = cn * cn;
+        auto paral_kernel_some = [&](cl::sycl::handler& cgh) {


Kernel inside the loop with bigger trip count. It would be more efficient to parallelize (make kernel) the algorithm by bigger value size instead size-n. So, it will require a loop inside the kernel.
It is questionable what will be more performant

loop with a kernels queue (data dependent)

kernel with a loop

It is hard to predict it with no perf measurements but I would vote that parallelization with bigger number of threads should be better.

shssf · 2021-06-22T17:38:31Z

@samir-nasibli Is this PR ready to review or still in development stage?

samir-nasibli · 2021-06-22T20:22:37Z

@samir-nasibli Is this PR ready to review or still in development stage?

I will update this PR or move some part of this changes to another PR with closing this.

Alexander-Makaryev · 2021-09-29T13:05:07Z

dpnp/backend/kernels/dpnp_krnl_random.cpp

+                    *n = *n + 1;
+                    result1[*n] = cl::sycl::asin(cl::sycl::sqrt(Y));


Looks like here we are getting race condition, that is why we are getting wrong results. To prevent it we should calculate n (index of result) from i.

Alexander-Makaryev · 2021-09-29T13:05:18Z

dpnp/backend/kernels/dpnp_krnl_random.cpp

+                    *n = *n + 1;
+                    result1[*n] = cl::sycl::acos(W);


Looks like here we are getting race condition, that is why we are getting wrong results. To prevent it we should calculate n (index of result) from i.

* Fix race condition in dpnp_rng_vonmises_small_kappa_c and dpnp_rng_vonmises_large_kappa_c * Rename arrays and change if condition from kernels in dpnp_rng_vonmises_large_kappa_c and dpnp_rng_vonmises_small_kappa_c * Add space * Fix indices in dpnp_rng_vonmises_small_kappa_c and dpnp_rng_vonmises_large_kappa_c

samir-nasibli · 2021-10-07T13:49:41Z

@LukichevaPolina
The use of extra memory with the amount of data is not a good practice in optimization. We must avoid this cases.
We have to remove the possibilities for a potential race condition in the algorithm.

densmirn · 2021-10-07T13:53:30Z

The use of extra memory with the amount of data is not a good practice in optimization. We must avoid this cases. - ideas?
We have to remove the possibilities for a potential race condition in the algorithm. - done

samir-nasibli · 2021-10-07T14:03:31Z

The use of extra memory with the amount of data is not a good practice in optimization. We must avoid this cases. - ideas?
We have to remove the possibilities for a potential race condition in the algorithm. - done

Any optimization with the use of additional memory can actually degrade (depending on the input data) and underestimate all the benefits from parallelism. Allocation/Deallocation/Working with additional memory is expensive.
We have to remove the possibilities for a potential race condition in the algorithm. - done

using extra mem is brute force approach.

- ideas?

We need to investigate it.

I also recommend to use perf tests and profiler tools during optimization. Comparative analysis is important in such work.

oleksandr-pavlyk · 2022-05-06T16:43:47Z

Stale PR?

ENH: kernels for random.vonmisses

63eeab1

samir-nasibli added the in progress Please do not merge. Work is in progress. label Apr 14, 2021

samir-nasibli added 2 commits April 14, 2021 15:18

update

5e6086c

refactoring

4268517

samir-nasibli removed the in progress Please do not merge. Work is in progress. label Apr 14, 2021

samir-nasibli requested a review from shssf April 14, 2021 20:27

samir-nasibli added the in progress Please do not merge. Work is in progress. label Apr 17, 2021

samir-nasibli added 4 commits April 21, 2021 16:26

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

6f77dc0

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

b5f539a

disabled tests on CPU

e9c17c7

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

75dc985

shssf reviewed May 13, 2021

View reviewed changes

samir-nasibli added 3 commits May 24, 2021 16:11

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

7221851

tmp solution

df3160f

revert last changes on dpnp_krnl_random.cpp

0e743d2

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

ff8de8e

samir-nasibli changed the title ~~ENH: kernels for random.vonmisses~~ ENH: kernels for random.vonmisses; part 2 Jul 12, 2021

samir-nasibli mentioned this pull request Jul 12, 2021

ENH: kernels for random.vonmisses; part 1 #779

Merged

samir-nasibli and others added 4 commits July 13, 2021 16:23

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

1492555

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

233cd59

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

30637c6

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

3222bc5

Alexander-Makaryev reviewed Sep 29, 2021

View reviewed changes

Alexander-Makaryev assigned LukichevaPolina Oct 1, 2021

Merge branch 'master' into samir-nasibli/enh/vonmisses_random

2c3eeb2

densmirn approved these changes Oct 7, 2021

View reviewed changes

antonwolfy marked this pull request as draft February 20, 2024 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: kernels for `random.vonmisses`; part 2 #681

ENH: kernels for `random.vonmisses`; part 2 #681

samir-nasibli commented Apr 14, 2021 •

edited

Loading

shssf May 13, 2021

samir-nasibli May 14, 2021

shssf May 13, 2021

shssf May 13, 2021 •

edited

Loading

shssf commented Jun 22, 2021

samir-nasibli commented Jun 22, 2021

Alexander-Makaryev Sep 29, 2021

Alexander-Makaryev Sep 29, 2021

samir-nasibli commented Oct 7, 2021 •

edited

Loading

densmirn commented Oct 7, 2021

samir-nasibli commented Oct 7, 2021

oleksandr-pavlyk commented May 6, 2022

		n = n + 1;
		result1[*n] = cl::sycl::asin(cl::sycl::sqrt(Y));

ENH: kernels for random.vonmisses; part 2 #681

Are you sure you want to change the base?

ENH: kernels for random.vonmisses; part 2 #681

Conversation

samir-nasibli commented Apr 14, 2021 • edited Loading

Description

Tests

TODO

shssf May 13, 2021

Choose a reason for hiding this comment

samir-nasibli May 14, 2021

Choose a reason for hiding this comment

shssf May 13, 2021

Choose a reason for hiding this comment

shssf May 13, 2021 • edited Loading

Choose a reason for hiding this comment

shssf commented Jun 22, 2021

samir-nasibli commented Jun 22, 2021

Alexander-Makaryev Sep 29, 2021

Choose a reason for hiding this comment

Alexander-Makaryev Sep 29, 2021

Choose a reason for hiding this comment

samir-nasibli commented Oct 7, 2021 • edited Loading

densmirn commented Oct 7, 2021

samir-nasibli commented Oct 7, 2021

oleksandr-pavlyk commented May 6, 2022

ENH: kernels for `random.vonmisses`; part 2 #681

ENH: kernels for `random.vonmisses`; part 2 #681

samir-nasibli commented Apr 14, 2021 •

edited

Loading

shssf May 13, 2021 •

edited

Loading

samir-nasibli commented Oct 7, 2021 •

edited

Loading