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

Reduce by key, extends #1141 #2097

Merged
merged 20 commits into from Apr 22, 2016
Merged

Reduce by key, extends #1141 #2097

merged 20 commits into from Apr 22, 2016

Conversation

biddisco
Copy link
Contributor

This is an algorithm present in thrust:: but not included in N4409. It is useful for a number of algorithms that use stream compaction (common in visualization).

}

// for debugging
std::ostream &operator<<(std::ostream &os,
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see this being hidden behind a pp constant. Also, if this is enabled, it requires <iostreams> to be included.

@hkaiser
Copy link
Member

hkaiser commented Apr 17, 2016

May I ask to change the naming convention partially used (for instance for ReduceKeySeriesStates, ReduceStencilGeneration and various variable names)? For this project we have settled to use gnu_style_names for types, variables, and functions. We use CamelStyle for template parameters only. ALL_CAPS is used for macro names. We don't use Hungarian Notation either (prefixing variable names with its shortened type, i.e. no fStart, fEnd).

I'm sorry to bring this up as it might seem unrelated and demeaning to the work you are doing. We try however to keep the coding style in the library as uniform as possible.

@hkaiser
Copy link
Member

hkaiser commented Apr 17, 2016

If the new algorithm is meant to be used for non-asynchronous execution policies only (no task) I'd suggest to prevent it from compiling when invoked with one. This should also be documented.

OTOH, creating a simple wrapper for the case of asynchronous policies which launches the corresponding non-asynchronous version in a new thread would do the trick as well. This might also simplify the implementation of the non-asynchronous version.

@hkaiser
Copy link
Member

hkaiser commented Apr 17, 2016

You added extensive doxygen comments to enable the creation of the documentation for the new algorithms. In order for the documentation to be actually created though, you'd need to add the file to the build system. Several sports would need to be touched:

Let me know if you would like to have some help with this.

@biddisco
Copy link
Contributor Author

unrelated and demeaning
Indeed. However, I will grudgingly comply with the naming - and fix the doc generation.
The async execution I'll need to look at more closely.

@biddisco biddisco force-pushed the reduce_by_key branch 4 times, most recently from 6cebb5d to 51ef310 Compare April 18, 2016 21:44
@biddisco
Copy link
Contributor Author

I've fixed the async execution and corrected (I believe) all the offending variable names, docs etc.
This was rebased onto the latest master today, so once the tests have passed I would be happy to see it merged.

@biddisco biddisco force-pushed the reduce_by_key branch 2 times, most recently from f4a0a11 to 6b39cc0 Compare April 19, 2016 13:56
(boost::is_base_of<std::random_access_iterator_tag, category2>::value) ||
(boost::is_base_of<std::output_iterator_tag, category3>::value) ||
(boost::is_base_of<std::output_iterator_tag, category4>::value),
"iterators : Random_access for inputs and Output for outputs.");
Copy link
Member

Choose a reason for hiding this comment

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

Did you intend to use && instead of ||? Also, any iterator type except input_iterators can be used as an output iterator. What you probably want to write in order to check whether an iterator conforms to the output iterator concept is:

hpx::traits::is_output_iterator<OutIter>::value || 
    hpx::traits::is_forward_iterator<OutIter>::value 

We also have a

hpx::traits::is_random_access_iterator<RanIter>::value

which could be used instead (this also gets rid of the boost/type_traits facilities which we replaced everywhere else in the parallelism files)..

@biddisco biddisco force-pushed the reduce_by_key branch 4 times, most recently from 82319aa to ffe1277 Compare April 20, 2016 20:56
@hkaiser hkaiser merged commit 82bb1c2 into master Apr 22, 2016
@hkaiser hkaiser deleted the reduce_by_key branch April 22, 2016 12:11
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.

None yet

2 participants