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 A par_chunk_by method #925

Merged
merged 12 commits into from
Mar 24, 2024
Merged

Added A par_chunk_by method #925

merged 12 commits into from
Mar 24, 2024

Conversation

jakeKonrad
Copy link
Contributor

Hello, I found this method useful in my code and thought it could be used here. However it relies on #![feature(slice_group_by)] unstable feature of the rust std library and you would probably want to wait for that method to be stabilized anyways so as to stay consistent with whatever naming they choose. This pull request is here for when that happens. Also if any one has any comments on the code I'd really appreciate hearing them.

@cuviper
Copy link
Member

cuviper commented Apr 6, 2022

Thanks, it's an interesting feature! Do you think you'll try a mutable version as well?

And yes, we'll want to wait for the API to stabilize in the standard library so we can match it.

@jakeKonrad
Copy link
Contributor Author

That should be the mutable version.

@jakeKonrad jakeKonrad marked this pull request as ready for review November 2, 2022 17:29
@cuviper
Copy link
Member

cuviper commented Nov 11, 2022

Just for reference, that slice_group_by feature is tracked in rust-lang/rust#80552.

@stevenliebregt
Copy link

I'm interested in this, but I found a bug in it, the following call:

let data = vec![1, 1, 2, 2, 2, 2, 2, 3, 3, 3, 4, 4];
let groups: Vec<_> = data.par_group_by(|&a, &b| a == b).collect::<Vec<_>>();

println!("{:?}", groups);

will result in 5 groups instead of the expected 4, these groups are:

[[1, 1], [2, 2, 2, 2, 2], [3], [3, 3], [4, 4]]

@jakeKonrad
Copy link
Contributor Author

I think that fixed it

@stevenliebregt
Copy link

I think that fixed it

It did, nice job!

@cuviper
Copy link
Member

cuviper commented Jan 26, 2024

FYI, that stabilization just landed with a rename to chunk_by / chunk_by_mut:
rust-lang/rust#117678

Copy link
Member

@cuviper cuviper left a comment

Choose a reason for hiding this comment

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

I wonder if this can be refactored to use the internal SplitProducer infrastructure, because that's basically what this iterator is doing overall, splitting on a pair-predicate. That API won't work directly as-is, but I've also been working on modifications to add split_inclusive, which might be instructive on how to extend it.

src/slice/group_by.rs Outdated Show resolved Hide resolved
@cuviper cuviper changed the title Added A par_group_by method Added A par_chunk_by method Mar 22, 2024
@cuviper cuviper changed the title Added A par_chunk_by method Added A par_chunk_by method Mar 22, 2024
In particular, this now ensures we only call the predicate once on
each pair of items.
@cuviper
Copy link
Member

cuviper commented Mar 23, 2024

I wonder if this can be refactored to use the internal SplitProducer infrastructure,

I didn't find a good way to shoehorn this into SplitProducer, per se, but I've now made the implementation quite similar to that, and a test asserts that we get exactly len() - 1 comparisons.

@cuviper cuviper added this pull request to the merge queue Mar 24, 2024
Merged via the queue into rayon-rs:main with commit ac2fa4d Mar 24, 2024
4 checks passed
@cuviper
Copy link
Member

cuviper commented Mar 24, 2024

Thanks @jakeKonrad! Hope you don't mind that I wrapped this up myself.

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