Add InvariantSemigroupal and ability to turn Monoidals to Monoids #2088

LukaJCB · 2017-12-10T01:12:57Z

Adds InvariantSemigroupal as supertype of InvariantMonoidal and just like with Apply and Applicative we have functions that turn invariant and contravariant Monoidal and Semigroupal Functors into Monoids and Semigroups respectively.
I'll try to add tests for these, as well as the ones in Apply and Applicative soon.

codecov-io · 2017-12-10T02:22:42Z

Codecov Report

Merging #2088 into master will increase coverage by 0.03%.
The diff coverage is 88.73%.

@@            Coverage Diff             @@
##           master    #2088      +/-   ##
==========================================
+ Coverage   94.63%   94.66%   +0.03%     
==========================================
  Files         325      328       +3     
  Lines        5514     5533      +19     
  Branches      221      199      -22     
==========================================
+ Hits         5218     5238      +20     
+ Misses        296      295       -1

Impacted Files	Coverage Δ
core/src/main/scala/cats/Semigroupal.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/Apply.scala	`75% <ø> (+6.25%)`	⬆️
core/src/main/scala/cats/Applicative.scala	`95% <0%> (+5%)`	⬆️
core/src/main/scala/cats/data/IndexedStateT.scala	`89.24% <0%> (-0.12%)`	⬇️
...rc/main/scala/cats/instances/partialOrdering.scala	`77.77% <100%> (+2.77%)`	⬆️
core/src/main/scala/cats/data/Kleisli.scala	`97.89% <100%> (ø)`	⬆️
.../cats/laws/discipline/InvariantMonoidalTests.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/WriterT.scala	`93.13% <100%> (ø)`	⬆️
...e/src/main/scala/cats/instances/partialOrder.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Tuple2K.scala	`95.34% <100%> (ø)`	⬆️
... and 27 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 02b819c...ff0a355. Read the comment docs.

kailuowang · 2017-12-11T19:20:55Z

core/src/main/scala/cats/Composed.scala

+  def G: Apply[G]
+
+  def product[A, B](fa: F[G[A]], fb: F[G[B]]): F[G[(A, B)]] =
+    F.imap(F.product(fa, fb)) { case (ga, gb) =>


do you plan to add tests for this instance?

kailuowang · 2017-12-11T19:39:20Z

core/src/main/scala/cats/InvariantSemigroupal.scala

+  */
+@typeclass trait InvariantSemigroupal[F[_]] extends Semigroupal[F] with Invariant[F] { self =>
+
+   def composeApply[G[_]: Apply]: InvariantSemigroupal[λ[α => F[G[α]]]] =


Any plan to test cover this one as well?

Has this been addressed?

kailuowang · 2017-12-11T19:41:20Z

core/src/main/scala/cats/InvariantSemigroupal.scala

+}
+
+object InvariantSemigroupal extends SemigroupalArityFunctions {
+  def semigroup[F[_], A](implicit f: InvariantSemigroupal[F], sg: Semigroup[A]): Semigroup[F[A]] =


The diff coverage is a bit low, one thing we could do is to add doctests for this semigroup method and the one in ContravariantSemitgroupal, which could also serve as examples. Not critically important though.

kailuowang · 2017-12-11T19:42:14Z

Overall looking good to me. Could use a couple of more tests for the composed instances to bring up the test coverage.

LukaJCB · 2017-12-11T22:34:16Z

Yeah this is still a WIP, wanted to add a bunch more tests, for Applicative/Apply as well.

LukaJCB · 2017-12-12T12:14:35Z

The more I work with this the more I think Apply and ContravariantSemigroupal should extend InvariantSemigroupal and the same applies to Applicative and ContravariantMonoidal.
It would definitely simplify a lot of the compositional things. :)

LukaJCB · 2017-12-12T15:10:44Z

I moved the semigroup and monoid methods to the type classes instead.
This gives us the benefit, that we can optimize this for any instance.

LukaJCB · 2017-12-12T16:44:21Z

So the version right now works without breaking changes, but it'd probably be cleaner to define InvariantMonoidal in terms of InvariantSemigroupal + def unit: F[Unit] and deriving pure from there.
Another thing that bothers me a bit is that I think every Semigroupal is bound to also be an Invariant Functor, no? Maybe I'm wrong about this, but I don't think it's possible. It would also simplify law testing because we can now imap over the resulting F.

kailuowang · 2017-12-12T17:24:02Z

InvariantMonoidal in terms of InvariantSemigroupal + def unit: F[Unit] and deriving pure from there.

Then Applicative won't be able to inherit from InvariantMonoidal right?

LukaJCB · 2017-12-12T17:26:48Z

Nope, that's no problem, in fact if you look at Applicative right now, it already derives unit from pure:

trait Applicative[F[_]] extends Apply[F] with InvariantMonoidal[F] {
  def pure[A](a: A): F[A]
  def unit: F[Unit] = pure(())
}

https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Applicative.scala#L46

kailuowang · 2017-12-12T17:30:00Z

hmm, I admit I am a bit confused, in ContravariantMonoidal the unit type is
def unit[A]: F[A]. It might not be able to define another def unit: F[Unit] ?

LukaJCB · 2017-12-12T17:34:07Z

It would have to be changed or renamed, yeah :/

Something like:

trait ContravariantMonoidal[F[_]] extends ContravariantSemigroupal[F] with InvariantMonoidal[F] {
  def conquer[A]: F[A]
  def unit: F[Unit] = conquer[Unit] 
}

Or we just remove it, since you can contramap to any type from an F[Unit] by simply doing val fa: F[A] = F.unit.contramap(_ => ())

LukaJCB · 2017-12-12T18:01:43Z

Another argument I find compelling is that literature always only refers to Semigroupal or Monoidal as Functors. i.e. Lax Monoidal Functor, Semigroupal Endofunctor.
So having a Semigroupal that's not a Functor doesn't seem to make a lot of sense naming wise, I think.

LukaJCB · 2017-12-16T21:36:53Z

Everything should be addressed :)

ceedubs

👍 LGTM. Codecov is reporting a drop in coverage which might be worth checking out.

kailuowang · 2017-12-18T01:11:31Z

core/src/main/scala/cats/Composed.scala

+    F.imap(F.product(fa, fb)) { case (ga, gb) =>
+      G.map2(ga, gb)(_ -> _)
+    } { g: G[(A, B)] =>
+      (G.map(g)(_._1), G.map(g)(_._2))


This line is not tested, we might need to test an instance that is not a covariant.

kailuowang · 2017-12-18T01:18:53Z

laws/src/main/scala/cats/laws/InvariantMonoidalLaws.scala


  def invariantMonoidalLeftIdentity[A, B](fa: F[A], b: B): IsEq[F[A]] =
-    F.pure(b).product(fa).imap(_._2)(a => (b, a)) <-> fa
+    F.point(b).product(fa).imap(_._2)(a => (b, a)) <-> fa


We can express these laws in unit right? like

def invariantMonoidalLeftIdentity[A, B](fa: F[A]): IsEq[F[A]] = F.unit.product(fa).imap(_._2)(a => ((), a)) <-> fa

Then we can add a point - unit consistency law. WDYT?

kailuowang · 2017-12-18T01:19:29Z

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

  implicit def catsConstInvariantMonoidal[C: Monoid]: InvariantMonoidal[Const[C, ?]] = new InvariantMonoidal[Const[C, ?]] {
-    def pure[A](a: A): Const[C, A] =
+    def unit: Const[C, Unit] =
      Const.empty


This one is not tested. I suspect that the instance was never tested before?

kailuowang · 2017-12-18T01:21:31Z

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

-  override def unit[A]: IndexedStateT[F, S, S, A]  =
-    IndexedStateT.applyF(G.pure((s: S) => F.unit[(S, A)]))
+  override def unit: IndexedStateT[F, S, S, Unit]  =
+    IndexedStateT.applyF(G.pure((s: S) => F.trivial[(S, Unit)]))


It's reported that this instance is not tested. Shall we add one?

the ContravariantMonoidal instance of IndexedStateT requires that F to have both FlatMap and ContravriantMonoidal. We can't think of any practical data types that satisfies that.

I incline to remove this instance, WDYT @stephen-lazaro ?

Agreed, it seems at best awkward and unuseful.

kailuowang · 2017-12-18T01:24:09Z

core/src/main/scala/cats/instances/eq.scala

       * contracting the type to a point
       */
-      def unit[A]: Eq[A] = Eq.allEqual
+      def unit: Eq[Unit] = Eq.allEqual


not quite sure I understand why this one is reported as not tested either. Any idea?

into LukaJCB-add-monoidal-to-monoid

Luka jcb add monoidal to monoid

LukaJCB · 2017-12-18T20:57:30Z

Sorry @kailuowang, needed to add a couple more mima exceptions

kailuowang · 2017-12-18T21:47:19Z

close and reopen to trigger build and recheck of conflict

Add InvariantSemigroupal and ability to turn Monoidals to Monoids

ec1f604

stew added the in progress label Dec 10, 2017

kailuowang added the enhancement label Dec 10, 2017

kailuowang added this to the 1.0.0 milestone Dec 10, 2017

Luka Jacobowitz added 3 commits December 10, 2017 11:31

Add Tests

2edcb85

Add composition with Apply

495cc74

Add Mima exception

d8b496d

kailuowang reviewed Dec 11, 2017

View reviewed changes

LukaJCB added the Binary Breaking label Dec 12, 2017

LukaJCB force-pushed the add-monoidal-to-monoid branch from 6d44ab1 to bd825fe Compare December 12, 2017 12:01

More tests + Nested

19dc0ff

LukaJCB force-pushed the add-monoidal-to-monoid branch from bd825fe to 19dc0ff Compare December 12, 2017 12:02

Remodel hierarchy

38b46e6

LukaJCB force-pushed the add-monoidal-to-monoid branch from 2e12089 to 3a1f937 Compare December 12, 2017 15:30

Move monoid and semigroup functions to the instance

f4b7490

LukaJCB force-pushed the add-monoidal-to-monoid branch from 3a1f937 to f4b7490 Compare December 12, 2017 15:41

More Tests

f9df1c3

LukaJCB added 2 commits December 16, 2017 19:33

Merge branch 'master' into add-monoidal-to-monoid

dfbcf66

Merge branch 'master' into add-monoidal-to-monoid

7880ada

ceedubs previously approved these changes Dec 17, 2017

View reviewed changes

LukaJCB requested a review from kailuowang December 17, 2017 17:21

kailuowang reviewed Dec 18, 2017

View reviewed changes

kailuowang and others added 4 commits December 18, 2017 10:42

Merge branch 'add-monoidal-to-monoid' of https://github.com/LukaJCB/cats

3e3982f

into LukaJCB-add-monoidal-to-monoid

use uinit for laws

95f58ff

improve test coverage

4b73af8

Merge pull request #3 from kailuowang/LukaJCB-add-monoidal-to-monoid

30391e8

Luka jcb add monoidal to monoid

LukaJCB dismissed ceedubs’s stale review via 30391e8 December 18, 2017 20:29

kailuowang previously approved these changes Dec 18, 2017

View reviewed changes

Add more mima exceptions

18f353d

LukaJCB dismissed kailuowang’s stale review via 18f353d December 18, 2017 20:56

Mima

436e444

kailuowang previously approved these changes Dec 18, 2017

View reviewed changes

Merge branch 'master' into add-monoidal-to-monoid

ff0a355

kailuowang dismissed their stale review via ff0a355 December 18, 2017 21:39

kailuowang closed this Dec 18, 2017

kailuowang reopened this Dec 18, 2017

kailuowang added the in progress label Dec 18, 2017

kailuowang approved these changes Dec 18, 2017

View reviewed changes

kailuowang merged commit 8d4d0b7 into typelevel:master Dec 18, 2017

kailuowang removed the in progress label Dec 18, 2017

Uh oh!

Add InvariantSemigroupal and ability to turn Monoidals to Monoids #2088

Add InvariantSemigroupal and ability to turn Monoidals to Monoids #2088

Uh oh!

Conversation

LukaJCB commented Dec 10, 2017

Uh oh!

codecov-io commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaJCB commented Dec 11, 2017

Uh oh!

LukaJCB commented Dec 12, 2017

Uh oh!

LukaJCB commented Dec 12, 2017

Uh oh!

LukaJCB commented Dec 12, 2017

Uh oh!

kailuowang commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LukaJCB commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kailuowang commented Dec 12, 2017

Uh oh!

LukaJCB commented Dec 12, 2017

Uh oh!

LukaJCB commented Dec 12, 2017

Uh oh!

LukaJCB commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ceedubs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kailuowang Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LukaJCB commented Dec 18, 2017

Uh oh!

kailuowang commented Dec 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

codecov-io commented Dec 10, 2017 •

edited

Loading

kailuowang commented Dec 11, 2017 •

edited

Loading

kailuowang commented Dec 12, 2017 •

edited

Loading

LukaJCB commented Dec 12, 2017 •

edited

Loading

LukaJCB commented Dec 16, 2017 •

edited

Loading

kailuowang Dec 18, 2017 •

edited

Loading

kailuowang Dec 18, 2017 •

edited

Loading