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

Add a par_fold method to Zip to improve the discoverability of Rayon's fold-reduce idiom. #1095

Merged
merged 1 commit into from
Nov 6, 2021

Conversation

adamreichold
Copy link
Collaborator

Fixes #1094

Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

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

Seems good - par equivalent of existing Zip::fold. I don't want us to forward everything from parallel iterators to Zip, so let's be conservative with that. It is written in the docs how to use those methods already.

/// let a = Array::<usize, _>::ones((128, 1024));
/// let b = Array::<usize, _>::ones(128);
///
/// let sum = Zip::from(a.rows()).and(b.view()).par_fold(
Copy link
Member

@bluss bluss Oct 30, 2021

Choose a reason for hiding this comment

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

Suggested change
/// let sum = Zip::from(a.rows()).and(b.view()).par_fold(
/// let weighted_sum = Zip::from(a.rows()).and(&b).par_fold(

weighted_sum is just a suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

You could if you want sync up even more with the existing Zip::fold in terms of "presentation". By no means necessary, just saying what I'm thinking - (see existing ones to see that they are not perfect). If you want, we mention something like par for each's This is a shorthand for using .into_par_iter().for_each() on Zip.

@adamreichold
Copy link
Collaborator Author

adamreichold commented Oct 30, 2021

I don't want us to forward everything from parallel iterators to Zip, so let's be conservative with that.

I do agree which is why I only went for the most common combination of fold immediately followed by reduce. But since my main goal here is discoverability, I think that I should definitely add

If you want, we mention something like par for each's This is a shorthand for using .into_par_iter().for_each() on Zip.

I do have one question concerning you suggestion though, in

let weighted_sum = Zip::from(&a).and(&b).par_fold(

you removed the a.rows() call. Was this intentional? My intention for the example was to avoid showing element-wise parallelism to place the idea in the readers mind that Zip and hence the parallel fold should often operate on larger chunks of the underlying data. Should I make that thought explicit in the doc comment?

@adamreichold
Copy link
Collaborator Author

adamreichold commented Oct 30, 2021

Should I make that thought explicit in the doc comment?

Added a suggestion on how this could be worded. I am not too invested in that though as there are too many things to consider in each particular situation so that such general advice might not apply.

@bluss
Copy link
Member

bluss commented Oct 30, 2021

Oh, I'm sorry about removing the .rows() thing - I was reading way too fast, thought it was .view() too.

@bluss bluss merged commit 493674d into rust-ndarray:master Nov 6, 2021
@bluss bluss added this to the 0.15.4 milestone Nov 6, 2021
@adamreichold adamreichold deleted the par-fold branch November 6, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to do parallel array .sum?
2 participants