Skip to content
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

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

Merged
merged 1 commit into from
Jul 12, 2021
Merged

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

merged 1 commit into from
Jul 12, 2021

Conversation

samir-nasibli
Copy link
Contributor

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
Copy link
Contributor 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
dpnp/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) {
Copy link
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.

Copy link
Contributor 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)

Copy link
Contributor 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 master Jul 12, 2021
@shssf shssf deleted the samir-nasibli/enh/update_random_vonmises branch July 12, 2021 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shssf shssf shssf approved these changes

Assignees

@shssf shssf

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@samir-nasibli @shssf