Add CommutativeApply and CommutativeApplicative #1927

LukaJCB · 2017-09-24T15:52:13Z

Had a quick discussion about this on gitter. Also adds an instance for Validated[E: CommutativeSemigroup, ?] :)

…mmutativeSemigroup, ?]

codecov-io · 2017-09-24T16:23:04Z

Codecov Report

Merging #1927 into master will decrease coverage by 0.08%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1927      +/-   ##
==========================================
- Coverage   96.21%   96.13%   -0.09%     
==========================================
  Files         272      278       +6     
  Lines        4628     4655      +27     
  Branches      124      130       +6     
==========================================
+ Hits         4453     4475      +22     
- Misses        175      180       +5

Impacted Files	Coverage Δ
...rc/main/scala/cats/laws/CommutativeMonadLaws.scala	`100% <ø> (ø)`	⬆️
.../main/scala/cats/laws/CommutativeFlatMapLaws.scala	`66.66% <ø> (ø)`	⬆️
core/src/main/scala/cats/CommutativeApply.scala	`0% <0%> (ø)`
core/src/main/scala/cats/CommutativeMonad.scala	`0% <0%> (ø)`	⬆️
core/src/main/scala/cats/CommutativeFlatMap.scala	`0% <0%> (ø)`	⬆️
...e/src/main/scala/cats/CommutativeApplicative.scala	`0% <0%> (ø)`
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	⬆️
...n/scala/cats/laws/CommutativeApplicativeLaws.scala	`100% <100%> (ø)`
.../laws/discipline/CommutativeApplicativeTests.scala	`100% <100%> (ø)`
...rc/main/scala/cats/laws/CommutativeApplyLaws.scala	`100% <100%> (ø)`
... and 11 more

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 b66b121...d673655. Read the comment docs.

LukaJCB · 2017-09-24T17:02:58Z

core/src/main/scala/cats/CommutativeApplicative.scala

+  *
+  * Must obey the laws defined in cats.laws.CommutativeApplicativeLaws.
+  */
+@typeclass trait CommutativeApplicative[F[_]] extends Applicative[F] with CommutativeApply[F]


For some reason Codecov doesn't think this is being hit, does anyone know why?

johnynek · 2017-09-24T18:09:37Z

This seems like a very interesting property (one that Option springs to mind as a canonical example), but chat can I do with an instance?

Can we not write a method or two to show the value of the abstraction. I tend to think to make it into cats an abstraction needs to pay its way. I haven’t yet seen that here, but probably because I am unfamiliar.

tpolecat · 2017-09-24T18:40:21Z

Future is another effect with this property. The order in which you construct futures is significant if you're side-effecting, but the order of applicative composition doesn't matter. Also the "sbt effect": the rewrite macro pulls the out the .value symbols in an arbitrary order and it doesn't matter.

You could use this property to properly constrain general rewrite systems like monadless (which preserves equational reasoning only for commutative monads).

None of these observations leads us closer to an easy example though. @edmundnoble mentioned that it might be handy for optimizing free applicatives since you could re-order effects.

edmundnoble · 2017-09-25T14:22:17Z

@johnynek Given the ability to reorder computations in FreeApplicative, you can batch them safely.

Also, unordered containers can be traversed safely with CommutativeApplicative, e.g. Set and Map. Right now, if you want to traverse them, you are forced into observing the implementation-defined ordering, and violating the contract of equality.

johnynek · 2017-09-28T00:38:30Z

those are good examples.

Could we add the methods to this PR?

LukaJCB · 2017-09-28T09:12:30Z

Hmm, so as I see we could either create some sort of traverseUnordered as syntax sugar on Set and Map, or we could want to create a type class for such unordered containers?

Leaning towards the former, as the latter might open another larger can of worms (Would a TraverseUnordered inherit from a theoretical non-structure preserving Functor?)

On another note, once #1837 is merged, I'm pretty sure all the Zip{List, Vector...} are all instances of CommutativeApplicative :)

LukaJCB · 2017-09-28T15:59:30Z

Okay, maybe Future is not a valid CommutativeApplicative because the ordering matters if one of the Futures is a Failure, so it only works for Successful cases.

This reverts commit 0cd4eb9.

tpolecat · 2017-09-28T19:04:41Z

@LukaJCB What was the issue with Future? (a *> b) and (b <* a) should always compute the same result.

LukaJCB · 2017-09-28T19:52:14Z

Yeah, but only if the result is successful:

scala> val a: Future[Int] = Future.failed(new Exception("A"))
a: scala.concurrent.Future[Int] = Future(Failure(java.lang.Exception: A))

scala> val b: Future[Int] = Future.failed(new Exception("B"))
b: scala.concurrent.Future[Int] = Future(Failure(java.lang.Exception: B))

scala> a *> b
res0: scala.concurrent.Future[Int] = Future(Failure(java.lang.Exception: A))

scala> b <* a
res1: scala.concurrent.Future[Int] = Future(Failure(java.lang.Exception: B))

tpolecat · 2017-09-28T20:12:57Z

@LukaJCB hell. Can't ever win with Future. Good counterexample, thanks!

LukaJCB · 2017-09-28T20:21:34Z

You really can't 😄. Want to weigh in on syntax sugar vs type class? :)

LukaJCB · 2017-09-30T18:35:42Z

Another cool thing is that CommutativeApplicative also composes using Nested and Tuple2K

kailuowang · 2017-10-02T16:59:59Z

core/src/main/scala/cats/data/Nested.scala

  implicit def catsDataInvariantForNestedContravariant[F[_]: Invariant, G[_]: Contravariant]: Invariant[Nested[F, G, ?]] =
    new NestedInvariant[F, G] {
      val FG: Invariant[λ[α => F[G[α]]]] = Invariant[F].composeContravariant[G]
    }
 }

+private[data] sealed abstract class NestedInstances11 extends NestedInstances12 {
+  implicit def catsDataCommutativeApplyForNestedContravariant[F[_]: CommutativeApply, G[_]: CommutativeApply]: CommutativeApply[Nested[F, G, ?]] =


Usually we place more specific instance higher priority, i.e. CommutativeApply and CommutativeApplicative should have higher priority than Apply and Applicative.
CommutativeApplicative should have higher priority than CommutativeApply.

Also line 53 the instance name was wrong catsDataContravariantForNested -> catsDataFunctorForNested do you mind fix that in this PR as well?

johnynek · 2017-10-18T19:25:29Z

core/src/main/scala/cats/CommutativeApplicative.scala

+  *
+  * Must obey the laws defined in cats.laws.CommutativeApplicativeLaws.
+  */
+@typeclass trait CommutativeApplicative[F[_]] extends Applicative[F] with CommutativeApply[F] {


I wonder if we really want an UnorderedFoldable and UnorderedTraverse that instead of toList has toSet and has a foldMap that takes a CommutativeMonoid and for sequence/traverse uses CommutativeApplicative.

That seems cleaner to me than putting these methods here now that I think about it.

I think we can construct sane laws for UnorderedFoldable that would make sense. If we added a method like insert[T](f: F[T], item: T): F[T] then I think you could make some law.

I agree that an extra type class would feel a lot cleaner than baking these 4 operations into the CommutativeApplicative type class. Related thoughts here.
I've played around with some laws, and we wouldn't have to get rid of too many laws too make it fit. I also like the insert, although I'd have to think about it more. We could let Foldable extend UnorderedFoldable, if that makes any sense. I'll play around with it and update the PR accordingly :)

This opens up a can of worms though. We could probably also define something like unorderedFoldM with CommutativeMonad, a UnorderedReducible, NonEmptyUnordered and all kinds of others. I came up with something crazy a while ago on gitter https://gitter.im/typelevel/cats-dev?at=59cf81a5210ac269208e0bcf but seems I've only scratched the surface 😄

I'll try to define this as minimal as possible and go from there. Might make a good blog post though.

I had to remove insert for now, because it's not possible to implement it for Map :/

…ons there

kailuowang · 2017-10-19T14:49:14Z

I have a proposal:
CommunitiveApply and CommutativeApplicative are obviously useful, straightforward and clearly defined. Can we limit this PR to these two type classes (without the unorderedTraverseXXX methods) and move all the unordered traverse stuff to a separate follow up PR?

LukaJCB · 2017-10-19T16:58:55Z

Yeah I can do that :)

…l functions there" This reverts commit d308cf7.

kailuowang · 2017-10-19T17:53:37Z

tests/src/test/scala/cats/tests/MapSuite.scala

@@ -15,6 +16,28 @@ class MapSuite extends CatsSuite {
  checkAll("Map[Int, Int] with Option", TraverseTests[Map[Int, ?]].traverse[Int, Int, Int, Int, Option, Option])
  checkAll("Traverse[Map[Int, ?]]", SerializableTests.serializable(Traverse[Map[Int, ?]]))

+  test("traverseUnordered identity") {


you forgot to remove these tests

This reverts commit 0ffbc55.

LukaJCB · 2017-10-19T18:16:14Z

Moved the unordered stuff to #1981

kailuowang · 2017-10-19T18:24:32Z

thanks so much! @LukaJCB

kailuowang · 2017-10-20T13:48:56Z

@johnynek this PR is simplified with only the Communtative type classes. Any other comments/feedbacks? Shall we merge this and then we can move to #1981?

johnynek

Thanks @LukaJCB !

Add CommutativeApply and Applicative and an instance for Validated[Co…

0ea4ac0

…mmutativeSemigroup, ?]

LukaJCB force-pushed the add-commutative-apply branch from 8c95312 to 0ea4ac0 Compare September 24, 2017 16:23

LukaJCB commented Sep 24, 2017

View reviewed changes

LukaJCB force-pushed the add-commutative-apply branch 2 times, most recently from ef2c94b to edd6ba8 Compare September 28, 2017 11:44

Make Future a CommutativeApplicative

0cd4eb9

LukaJCB force-pushed the add-commutative-apply branch from edd6ba8 to 0cd4eb9 Compare September 28, 2017 12:14

Revert "Make Future a CommutativeApplicative"

f9a239a

This reverts commit 0cd4eb9.

Add unordered traversal for Sets

965f9c2

LukaJCB force-pushed the add-commutative-apply branch from 6c2a240 to fcb429d Compare September 30, 2017 18:34

LukaJCB force-pushed the add-commutative-apply branch from fcb429d to 5bf6f35 Compare September 30, 2017 20:18

Test commutativeApply instances + Nested + Tuple2K instances

d3a803a

LukaJCB force-pushed the add-commutative-apply branch from 5bf6f35 to d3a803a Compare October 2, 2017 16:22

kailuowang reviewed Oct 2, 2017

View reviewed changes

Luka Jacobowitz added 3 commits October 2, 2017 19:17

Add unordered traversal for Map

1ab4f47

Fix priority for Nested instances

2164b51

Fix priority for Tuple2K instances

7f1f6c7

LukaJCB added the in progress label Oct 2, 2017

kailuowang previously approved these changes Oct 17, 2017

View reviewed changes

Try out new typeclasses

aa49de9

johnynek reviewed Oct 18, 2017

View reviewed changes

LukaJCB dismissed kailuowang’s stale review via d3f10ca October 18, 2017 22:50

Add UnorderedFoldable and UnorderedTraverse and move traversal functi…

d308cf7

…ons there

LukaJCB force-pushed the add-commutative-apply branch from d3f10ca to d308cf7 Compare October 18, 2017 22:51

Luka Jacobowitz added 3 commits October 18, 2017 19:39

Merge branch 'master' into add-commutative-apply

148c113

Remove Set from Foldable docs

0ffbc55

Add extra ref law

23cb589

Merge branch 'master' into add-commutative-apply

e0069a8

LukaJCB force-pushed the add-commutative-apply branch 2 times, most recently from 4889a52 to e0069a8 Compare October 19, 2017 17:40

Revert "Add UnorderedFoldable and UnorderedTraverse and move traversa…

376a5c6

…l functions there" This reverts commit d308cf7.

LukaJCB force-pushed the add-commutative-apply branch from 908c914 to 305d740 Compare October 19, 2017 17:52

kailuowang reviewed Oct 19, 2017

View reviewed changes

LukaJCB force-pushed the add-commutative-apply branch from 305d740 to 028b9a7 Compare October 19, 2017 17:55

Revert "Remove Set from Foldable docs"

d673655

This reverts commit 0ffbc55.

LukaJCB force-pushed the add-commutative-apply branch from 028b9a7 to d673655 Compare October 19, 2017 17:56

LukaJCB mentioned this pull request Oct 19, 2017

Add UnorderedFoldable and UnorderedTraverse #1981

Merged

kailuowang approved these changes Oct 19, 2017

View reviewed changes

kailuowang mentioned this pull request Oct 26, 2017

[Meta] PR sign-off policy #1994

Closed

johnynek approved these changes Oct 26, 2017

View reviewed changes

johnynek merged commit 8fd709a into typelevel:master Oct 26, 2017

stew removed the in progress label Oct 26, 2017

LukaJCB deleted the add-commutative-apply branch October 26, 2017 17:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CommutativeApply and CommutativeApplicative #1927

Add CommutativeApply and CommutativeApplicative #1927

LukaJCB commented Sep 24, 2017

codecov-io commented Sep 24, 2017 •

edited

Loading

LukaJCB Sep 24, 2017

johnynek commented Sep 24, 2017

tpolecat commented Sep 24, 2017 •

edited

Loading

edmundnoble commented Sep 25, 2017 •

edited

Loading

johnynek commented Sep 28, 2017

LukaJCB commented Sep 28, 2017 •

edited

Loading

LukaJCB commented Sep 28, 2017

tpolecat commented Sep 28, 2017 •

edited

Loading

LukaJCB commented Sep 28, 2017 •

edited

Loading

tpolecat commented Sep 28, 2017

LukaJCB commented Sep 28, 2017 •

edited

Loading

LukaJCB commented Sep 30, 2017

kailuowang Oct 2, 2017

johnynek Oct 18, 2017

LukaJCB Oct 18, 2017

LukaJCB Oct 18, 2017

LukaJCB Oct 18, 2017

kailuowang commented Oct 19, 2017

LukaJCB commented Oct 19, 2017

kailuowang Oct 19, 2017

LukaJCB commented Oct 19, 2017

kailuowang commented Oct 19, 2017

kailuowang commented Oct 20, 2017 •

edited

Loading

johnynek left a comment

Add CommutativeApply and CommutativeApplicative #1927

Add CommutativeApply and CommutativeApplicative #1927

Conversation

LukaJCB commented Sep 24, 2017

codecov-io commented Sep 24, 2017 • edited Loading

Codecov Report

LukaJCB Sep 24, 2017

Choose a reason for hiding this comment

johnynek commented Sep 24, 2017

tpolecat commented Sep 24, 2017 • edited Loading

edmundnoble commented Sep 25, 2017 • edited Loading

johnynek commented Sep 28, 2017

LukaJCB commented Sep 28, 2017 • edited Loading

LukaJCB commented Sep 28, 2017

tpolecat commented Sep 28, 2017 • edited Loading

LukaJCB commented Sep 28, 2017 • edited Loading

tpolecat commented Sep 28, 2017

LukaJCB commented Sep 28, 2017 • edited Loading

LukaJCB commented Sep 30, 2017

kailuowang Oct 2, 2017

Choose a reason for hiding this comment

johnynek Oct 18, 2017

Choose a reason for hiding this comment

LukaJCB Oct 18, 2017

Choose a reason for hiding this comment

LukaJCB Oct 18, 2017

Choose a reason for hiding this comment

LukaJCB Oct 18, 2017

Choose a reason for hiding this comment

kailuowang commented Oct 19, 2017

LukaJCB commented Oct 19, 2017

kailuowang Oct 19, 2017

Choose a reason for hiding this comment

LukaJCB commented Oct 19, 2017

kailuowang commented Oct 19, 2017

kailuowang commented Oct 20, 2017 • edited Loading

johnynek left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 24, 2017 •

edited

Loading

tpolecat commented Sep 24, 2017 •

edited

Loading

edmundnoble commented Sep 25, 2017 •

edited

Loading

LukaJCB commented Sep 28, 2017 •

edited

Loading

tpolecat commented Sep 28, 2017 •

edited

Loading

LukaJCB commented Sep 28, 2017 •

edited

Loading

LukaJCB commented Sep 28, 2017 •

edited

Loading

kailuowang commented Oct 20, 2017 •

edited

Loading