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

Added segmented fill for hpx::vector #2202

Merged
merged 10 commits into from Jun 15, 2016
Merged

Added segmented fill for hpx::vector #2202

merged 10 commits into from Jun 15, 2016

Conversation

@Sanac
Copy link
Contributor

@Sanac Sanac commented Jun 6, 2016

Adding segmented fill for hpx::vector. (refer to #1338)

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 7, 2016

This is great!

Could this be somehow merged with the segmented for_each? While it might be necessary, it would be nice if we could avoid reimplementing each of the algorithms separately.

@Sanac
Copy link
Contributor Author

@Sanac Sanac commented Jun 9, 2016

Rewritten so that segmented_fill uses for_each.

@biddisco
Copy link
Contributor

@biddisco biddisco commented Jun 9, 2016

I've not yet been through the code completely, but we discussed (via skype), that the tests should be extended to run distributed as well as locally. This will require some cmake additions that we'll put on another PR once they are done.

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 9, 2016

@biddisco What changes to the build/test system do you have in mind?

@biddisco
Copy link
Contributor

@biddisco biddisco commented Jun 9, 2016

Only some extra cmake code in tests/unit/component/CMakeLists.txt to add tests for partition_vector algorithms into distributed tests as well.

I believe that any partitioned_vector tests should be run with distributed as well as local partitions and I was not able to locate any existing tests of them like this.

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 9, 2016

I believe that any partitioned_vector tests should be run with distributed as well as local partitions and I was not able to locate any existing tests of them like this.

I agree. I'm just not sure what those tests would need in addition to the other distributed tests we already run.

return;
},
std::move(r)));
}
Copy link
Member

@hkaiser hkaiser Jun 13, 2016

@Sanac What is the rationale of separating the synchronous and asynchronous versions here. Wouldn't both be equivalent, letting the underlying parallel::for_each handle the differences?:

     template <typename Algo, typename ExPolicy, typename SegIter, typename T>  
     static typename util::detail::algorithm_result<ExPolicy, void>::type  
     fill_(ExPolicy && policy, SegIter first, SegIter last, T const& value)  
     {  
         return hpx::parallel::for_each(std::forward<ExPolicy>(policy),
              first, last, fill_function<T>(value));  
     } 

Copy link
Member

@hkaiser hkaiser Jun 13, 2016

@Sanac: well, it probably should be:

     template <typename Algo, typename ExPolicy, typename SegIter, typename T>  
     static typename util::detail::algorithm_result<ExPolicy, void>::type  
     fill_(ExPolicy && policy, SegIter first, SegIter last, T const& value)  
     {  
         typedef typename util::detail::algorithm_result<ExPolicy, void>::type  
             result_type;
         return hpx::util::void_guard<result_type>(),
             hpx::parallel::for_each(std::forward<ExPolicy>(policy),
                 first, last, fill_function<T>(value));
     } 

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 15, 2016

@Sanac thanks for those changes! One last question: what's the rationale of making the implementation functions for the new algorithm non-static?

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 15, 2016

@Sanac I take that back, you changed from inline to static, not v.v.
Everything looks good to me now, thanks!

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 15, 2016

@hkaiser
Copy link
Member

@hkaiser hkaiser commented Jun 15, 2016

Thanks!

@hkaiser hkaiser merged commit df35195 into STEllAR-GROUP:master Jun 15, 2016
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants