Skip to content

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

Merged
shssf merged 1 commit into
masterfrom
samir-nasibli/enh/update_random_vonmises
Jul 12, 2021
Merged

ENH: kernels for random.vonmisses; part 1#779
shssf merged 1 commit into
masterfrom
samir-nasibli/enh/update_random_vonmises

Conversation

@samir-nasibli

@samir-nasiblisamir-nasibli commented Jul 12, 2021

Copy link
Copy Markdown

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] PASSED
tests/test_random.py::TestDistributionsVonmises::test_seed[small_kappa] PASSED
  • + numpy external

@samir-nasibli

samir-nasibli commented Jul 12, 2021

Copy link
Copy Markdown
Author

@shssf moved some part of changes from #681 here. This PR is ready to merge.

shssf
shssf reviewed Jul 12, 2021
View reviewed changes
Comment threaddpnp/backend/kernels/dpnp_krnl_random.cpp
result1[i] = (resi < 0) ? -mod : mod;
}
cl::sycl::range<1> gws(size);
auto paral_kernel_acceptance = [&](cl::sycl::handler& cgh) {

@shssfshssfJul 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it looks like this lambda is mostly the same as in other part of code. I suspect, this will be used for many other part of code.
If it is not - please drop off this message.
So, perhaps it is better to make it as a macro or, that is better, as inline function in this C++ source. Just to avoid code duplication.
This is not quite important. It might be leaved as is, at least for now.

@samir-nasiblisamir-nasibliJul 12, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@shssf There is no code duplication here. Please read messages carefully)

@samir-nasiblisamir-nasibliJul 12, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@shssf Actually you are right. But it is valuable, when we reusing the same code more than 2 times. I vote leave it as is.

@shssf
shssf merged commit 3b6a339 into masterJul 12, 2021
@shssf
shssf deleted the samir-nasibli/enh/update_random_vonmises branch July 12, 2021 16:05
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@shssfshssfshssf approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@shssfshssf

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@samir-nasibli@shssf