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 unordered parallel variants of (flat)traverse and (flat)sequence #2946

Merged
merged 3 commits into from
Aug 5, 2019
Merged

add unordered parallel variants of (flat)traverse and (flat)sequence #2946

merged 3 commits into from
Aug 5, 2019

Conversation

mberndt123
Copy link
Contributor

Hi,

There currently doesn't seem to be a convenient way to traverse a Set with IO. traverse doesn't work because there's no Traverse instance for Set, and unorderedTraverse doesn't work because there is no CommutativeApplicative instance for IO. So I've added parUnorderedTraverse and parUnorderedSequence for that purpose.

I also added parUnorderedFlatTraverse and parUnorderedFlatSequence for symmetry, but given that the non-par versions of these don't exist I'm not really sure we need these.

Another logical set of operations would be parUnorderedTraverse_ and parUnorderedSequence_. But again, there is no unorderedTraverse_ or unorderedSequence_ in UnorderedFoldable, so it's not going to be any more efficient than the non-_ variants, and given that it's easy to just discard the return value I haven't added them.

So overall I think that parUnorderedTraverse and parUnorderedSequence should be pretty uncontroversial. Which of the other ones do you think we want? I think they all make sense for symmetry, but then I'm not sure how useful they are in practice.

Oh, and one more question: is FlatMap actually the right typeclass to use for the flat variants, or should it be CommutativeFlatMap?

@mberndt123
Copy link
Contributor Author

mberndt123 commented Jul 14, 2019

OK, somebody beat me to it: #2931

Too bad I didn't notice that earlier. But hey, you probably really want those flat variants, don't you? 😆

@codecov-io
Copy link

codecov-io commented Jul 14, 2019

Codecov Report

Merging #2946 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2946      +/-   ##
==========================================
- Coverage   93.99%   93.84%   -0.15%     
==========================================
  Files         369      369              
  Lines        7056     7067      +11     
  Branches      179      195      +16     
==========================================
  Hits         6632     6632              
- Misses        424      435      +11
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/parallel.scala 62.5% <0%> (-17.5%) ⬇️
core/src/main/scala/cats/Parallel.scala 84.37% <0%> (-5.63%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2cdd6d...6f4d1d6. Read the comment docs.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Sorry for the wait, this looks great :)
Can we add some tests to the SyntaxSuite to ensure these resolve properly?

@mberndt123
Copy link
Contributor Author

mberndt123 commented Aug 2, 2019

Hey @LukaJCB,

I've added some tests now. Thanks for the idea, it actually helped me shake out some silly mistakes in the implementation.

@mberndt123
Copy link
Contributor Author

I've also removed ParallelUnorderedFlatTraverseOps and moved its only method to ParallelUnorderedTraverseOps. I don't think there's a reason to have separate classes for those two methods.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

@kailuowang kailuowang added this to the 2.0.0-RC1 milestone Aug 5, 2019
@kailuowang kailuowang merged commit 28ecdcb into typelevel:master Aug 5, 2019
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.

5 participants