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.

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

Parallel version of random walks #172

Merged
merged 13 commits into from
Feb 21, 2022

Conversation

Kostas-Pallikaris
Copy link
Contributor

This PR provides multi-threaded version for the following random walks:

  • Random Directions Hit-and-Run (uniform and Gaussian sampling)
  • Coordinate Directions Hit-and-Run (uniform and Gaussian sampling)
  • Billiard walk
  • Random Directions Hit-and-Run for boundary sampling
  • Coordinate Directions Hit-and-Run for boundary sampling

The interface is written in C++ and uses the library openmp.

@vissarion
Copy link
Member

👋 @Kostas-Pallikaris what is the status of this PR? Is it ready for review?

@codecov
Copy link

codecov bot commented Feb 17, 2022

Codecov Report

Merging #172 (a1d47e6) into develop (6591ae8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #172   +/-   ##
========================================
  Coverage    55.44%   55.44%           
========================================
  Files           85       85           
  Lines         4801     4801           
  Branches      2111     2111           
========================================
  Hits          2662     2662           
  Misses         889      889           
  Partials      1250     1250           
Impacted Files Coverage Δ
include/random_walks/boundary_rdhr_walk.hpp 68.00% <ø> (ø)
include/random_walks/uniform_cdhr_walk.hpp 92.85% <ø> (ø)

@TolisChal
Copy link
Member

@Kostas-Pallikaris, thanks for the revised version!

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

I have some comments.

I would also suggest to include all walks in one file in include/random_walks e.g. multithread_walks.hpp.

It would be very useful to add an example that uses the new functionality as a use case.


omp_set_num_threads(num_threads);
unsigned int jj = 0, d= P.dimension(), m = P.num_of_hyperplanes;
std::vector<int> num_points_per_thread(num_threads, 0);
Copy link
Member

Choose a reason for hiding this comment

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

What about this

std::vector<int> num_points_per_thread(rnum%num_threads, rnum/num_threads+1);
std::vector<int> a(num_threads - rnum%num_threads, rnum/num_threads);
num_points_per_thread.insert(num_points_per_thread.end(), a.begin(), a.end());

instead of the double loop? Seems simpler and more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @vissarion. I've implemented this.

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

I am ok with merging now. Thanks!
I have one more minor comment

};

template <>
struct policy_storing<BCDHRWalk_multithread>
Copy link
Member

Choose a reason for hiding this comment

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

you do not have to repeat it here, just inherit the method from policy_storing<BRDHRWalk_multithread> i.e.

template <>
struct policy_storing<BCDHRWalk_multithread> : policy_storing<BRDHRWalk_multithread> 
{}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @vissarion. I've implemented this change.

@TolisChal TolisChal merged commit b60365d into GeomScale:develop Feb 21, 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.

3 participants