Add Representable Functor #2284

eli-jordan · 2018-06-07T12:19:28Z

I have been working on adding representable functors to cats.

This PR adds;

The Representable typeclass.
The RepresentableStore data type, which is a generalisation of the Store comonad.

codecov-io · 2018-06-07T13:04:45Z

Codecov Report

Merging #2284 into master will increase coverage by 0.02%.
The diff coverage is 98.11%.

@@            Coverage Diff             @@
##           master    #2284      +/-   ##
==========================================
+ Coverage   95.03%   95.05%   +0.02%     
==========================================
  Files         338      343       +5     
  Lines        5878     5931      +53     
  Branches      210      220      +10     
==========================================
+ Hits         5586     5638      +52     
- Misses        292      293       +1

Impacted Files	Coverage Δ
testkit/src/main/scala/cats/tests/CatsSuite.scala	`70% <ø> (ø)`	⬆️
core/src/main/scala/cats/instances/tuple.scala	`100% <100%> (ø)`	⬆️
...rc/main/scala/cats/laws/discipline/Arbitrary.scala	`98.96% <100%> (+0.05%)`	⬆️
core/src/main/scala/cats/data/package.scala	`88.88% <100%> (+1.38%)`	⬆️
...ore/src/main/scala/cats/syntax/representable.scala	`100% <100%> (ø)`
...s/src/main/scala/cats/laws/RepresentableLaws.scala	`100% <100%> (ø)`
...cala/cats/laws/discipline/RepresentableTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/instances/function.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Kleisli.scala	`97.05% <100%> (+0.12%)`	⬆️
core/src/main/scala/cats/Representable.scala	`100% <100%> (ø)`
... and 8 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 b77c388...c31445e. Read the comment docs.

johnynek

this looks interesting to me. I was not aware of it.

I'll try to think about the tailRecM.

johnynek · 2018-06-07T13:29:12Z

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

@@ -7,7 +7,8 @@ import cats.arrow._
 /**
 * Represents a function `A => F[B]`.
 */


can we revert the style changes in this file?

johnynek · 2018-06-07T13:33:07Z

core/src/main/scala/cats/Representable.scala

+ }
+ }
+
+ def representableIsBimonad[F[_], R](implicit R: Representable.Aux[F, R], M: Monoid[R]): Monad[F] = new Bimonad[F] with StackSafeMonad[F] {


don't we only get Monad if there is no Monoid[R]? Should we make that a lower priority implicit?

Yes thats right. I wasn't sure how to prioritise the implicits when I wrote that instance, but just read up on it. I added RepresentableInstances1 and RepresentableInstances0

johnynek · 2018-06-07T13:36:00Z

core/src/main/scala/cats/Representable.scala

+ R.tabulate(a => R.index(f(R.index(fa)(a)))(a))
+
+ // TODO: Can we make this monad stack safe?
+ // override def tailRecM[A, B](a: A)(f: A => F[Either[A, B]]): F[B] = ???


I think something like this works:

R.tabulate { r: R => @annotation.tailrec def loop(a: A): B = R.index(f(a))(r) match { case Right(b) => b case Left(a) => loop(a) } loop(a) }

Thanks! I hadn't got around to thinking this through. That solution works.

johnynek · 2018-06-07T13:38:33Z

tests/src/test/scala/cats/tests/MonadSuite.scala

@@ -23,7 +23,7 @@ class MonadSuite extends CatsSuite {
 forAll(smallPosInt) { (max: Int) =>
 val (result, aggregation) = incrementAndGet.whileM[Vector](StateT.inspect(i => !(i >= max))).run(0)
 result should ===(Math.max(0, max))
- aggregation should === ( if(max > 0) (1 to max).toVector else Vector.empty )
+ aggregation should === ( if (max > 0) (1 to max).toVector else Vector.empty )


can we revert style changes?

johnynek · 2018-06-07T13:39:00Z

tests/src/test/scala/cats/tests/KleisliSuite.scala

@@ -39,13 +39,13 @@ class KleisliSuite extends CatsSuite {
 checkAll("Semigroupal[Kleisli[Option, Int, ?]]", SerializableTests.serializable(Semigroupal[Kleisli[Option, Int, ?]]))



can we revert the style changes? I'd prefer a PR with style only changes so we can focus on logic/design.

johnynek · 2018-06-07T13:39:34Z

tests/src/test/scala/cats/tests/RepresentableSuite.scala

+
+ type Pair[A] = (A, A)
+ checkAll("Pair[String, String] <-> Boolean => String", RepresentableTests[Pair, Boolean].representable[String])
+


can we check the monad/comonad laws on a Representable here?

I added a check for Bimonad

eli-jordan · 2018-06-12T12:34:24Z

@johnynek @tpolecat @kailuowang I think this is ready for a final review / approval

johnynek · 2018-06-13T05:15:50Z

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

+ * A generalisation of the Store comonad, for any `Representable` functor.
+ * `Store` is the dual of `State`
+ */
+final case class RepresentableStore[F[_], S, A](fa: F[A])(val index: S)(implicit R: Representable.Aux[F, S]) {


I would rather not use the second parameter for index. I think scala does not use that for equality (only the first item is checked) and I doing think we want that. Also, I’d rather take R as an implicit parameter at the callsites that use it.

All methods in the class depend on the Representable instance, thats why I shifted it be a constructor parameter. The general reason (that I have seen) to move implicit constraints to methods rather than constructor params, it to avoid over constraining methods that do not require the implicit. I guess the downside is that if additional methods are added, that don't use the Representable instance, they will be over constrained. Is it worth moving the constraint in this case?

I will remove the second parameter list for index, it just made the implementation slightly cleaner.

kailuowang · 2018-06-19T19:52:49Z

@eli-jordan it looks like there are some duplicated commits created when you merge master to your branch. You might need to hard reset to master and cherry-pick cd82d38 to produce a clean PR.

kailuowang · 2018-06-19T20:41:17Z

core/src/main/scala/cats/Representable.scala

+}
+
+private[cats] sealed abstract class RepresentableInstances1 {
+ def catsRepresentableIsMonad[F[_], R](implicit Rep: Representable.Aux[F, R]): Monad[F] = new RepresentableMonad[F, R] {


This and the one above are not implicit and not much point to be imported and thus no need to follow the implicits naming convention. If we want them to be available for auto derivation of Bimonad and Monad we need to put them in a trait extendable by cats.implicits and better have some tests for potential conflicts. I vote for just rename them to better searchable if there aren't many real world use cases for them.

eli-jordan · 2018-06-20T12:22:24Z

@kailuowang I have made the suggested change, and cleaned up the commit history.

kailuowang · 2018-06-22T16:17:21Z

tests/src/test/scala/cats/tests/RepresentableSuite.scala

+
+ type Pair[A] = (A, A)
+
+ checkAll("Id[Int] <-> Unit => String", RepresentableTests[Id, Unit].representable[String])


another nit: Id[String]

Nice catch! :-)

kailuowang · 2018-06-22T17:20:26Z

tests/src/test/scala/cats/tests/RepresentableSuite.scala

+ implicit val bimonadInstance = Representable.bimonad[Pair, Boolean]
+ checkAll("Pair[Int]", BimonadTests[Pair].bimonad[Int, Int, Int])
+ checkAll("Bimonad[Pair]", SerializableTests.serializable(Bimonad[Pair]))
+ }


would you test the monad as well?

I didn't because BimonadTests runs the laws for both Monad and Comonad plus some others and the implementation is shared. Happy to add one for monad directly though.

Thanks for adding it. It wasn't a big deal, but just one more red spot on the coverage report that can always cost some attention in the future. Also it would be nice to have it covered in case
someone for whatever reason changes the monad implementation.

kailuowang · 2018-06-22T17:21:35Z

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

+ * Given a functorial computation on the index `S` peek at the value in that functor.
+ */
+ def experiment[G[_]](fn: S => G[S])(implicit G: Functor[G]): G[A] = {
+ G.map(fn(index))(peek)


mind to add a test for this method? I think a doctest would probably suffice.

eli-jordan · 2018-06-23T09:02:38Z

@kailuowang Thanks for the review. I have updated based on your comments.

LukaJCB · 2018-06-23T09:11:26Z

I'm super late to this PR and I apoligize for that. I've never used representable functors before, but I took a look at the Haskell package you mentioned at the top and I see that it implies a Distributive instead of just Functor, is there a reason why you didn't choose to use our Distributive instead? :)

eli-jordan · 2018-06-23T09:30:53Z

@LukaJCB Because you can actually derive distribute for any Representable

distributeRep :: (Representable f, Functor w) => w (f a) -> f (w a)
distributeRep wf = tabulate (\k -> fmap (`index` k) wf)

I guess I could have made trait Representable extends Distributive { .... But if I do that, should I do the same for Monad?

I added the derived Monad and Bimonad instance, but did not add the Distributive because I figured it could be added at a later date if desired.

LukaJCB · 2018-06-23T09:34:51Z

Sounds good 👍 We can add it later :)

kailuowang · 2018-06-25T14:41:35Z

Looking good to me. I am not very familiar with the practical use of it but the API looks clean and straightforward. Will be great to have two other review approvals.

kailuowang · 2018-06-27T11:13:41Z

@johnynek any other issues?

johnynek · 2018-07-03T15:14:29Z

core/src/main/scala/cats/syntax/representable.scala

+ def index(implicit R: Representable.Aux[F, R]): R => A = R.index(fa)
+}
+
+final class TabulateOps[A, R](f: R => A) {


Why not extend AnyVal here and above?

If there is a reason can we comment?

When I first wrote it, I had the ops class taking implicit params and AnyVal doesn't allow multiple param lists. I missed adding it back when I changed that, will fix that up now.

…to representable-functor

johnynek · 2018-07-05T22:48:49Z

core/src/main/scala/cats/syntax/representable.scala

+ new IndexOps[F, A, R](fa)
+}
+
+final class IndexOps[F[_], A, R](val fa: F[A]) extends AnyVal {


Should this have R on the class? Maybe it doesn’t matter but it seems like it should be on index.

In my tests it didn't seem to make a difference, but I think you're right, it makes more sense to put it on index. I'll make that change now.

johnynek

left a comment about possibly adding Representable for Eval.

johnynek · 2018-07-09T16:04:17Z

core/src/main/scala/cats/package.scala

@@ -77,6 +77,18 @@ package object cats {
 override def isEmpty[A](fa: Id[A]): Boolean = false
 }

+ /**
+ * Witness for: Id[A] <-> Unit => A


we could add something similar for Eval[A] potentially.

+1 , Since this has been blocking 1.2.0 release, I propose we tackle that in a separate PR
I added an issue #2320

LukaJCB

👍

johnynek reviewed Jun 7, 2018

View reviewed changes

eli-jordan changed the title ~~WIP: Add Representable Functor~~ Add Representable Functor Jun 9, 2018

eli-jordan changed the title ~~Add Representable Functor~~ WIP: Add Representable Functor Jun 11, 2018

eli-jordan force-pushed the representable-functor branch 2 times, most recently from 3b014ea to 7e54efb Compare June 12, 2018 11:37

eli-jordan changed the title ~~WIP: Add Representable Functor~~ Add Representable Functor Jun 12, 2018

johnynek reviewed Jun 13, 2018

View reviewed changes

kailuowang mentioned this pull request Jun 19, 2018

Add Defer typeclass, laws and implementations #2279

Merged

kailuowang added this to the 1.2 milestone Jun 19, 2018

kailuowang reviewed Jun 19, 2018

View reviewed changes

Add Representable Functor and RepresentableStore

b99e781

eli-jordan force-pushed the representable-functor branch from f9dd8af to b99e781 Compare June 20, 2018 11:40

kailuowang reviewed Jun 22, 2018

View reviewed changes

code review updates

0ff7eb9

kailuowang previously approved these changes Jun 25, 2018

View reviewed changes

Merge branch 'master' into representable-functor

c2c55ea

kailuowang dismissed their stale review via c2c55ea July 3, 2018 15:05

johnynek reviewed Jul 3, 2018

View reviewed changes

eli-jordan added 2 commits July 4, 2018 11:46

extend AnyVal for representable syntax

7f1c62f

Merge branch 'representable-functor' of github.com:eli-jordan/cats in…

cff12a6

…to representable-functor

kailuowang previously approved these changes Jul 5, 2018

View reviewed changes

johnynek reviewed Jul 5, 2018

View reviewed changes

updates to RepresentableSyntax

c31445e

eli-jordan dismissed kailuowang’s stale review via c31445e July 8, 2018 02:59

kailuowang approved these changes Jul 9, 2018

View reviewed changes

johnynek approved these changes Jul 9, 2018

View reviewed changes

kailuowang mentioned this pull request Jul 9, 2018

Representable instance for Eval #2320

Closed

kailuowang requested a review from LukaJCB July 9, 2018 16:42

LukaJCB approved these changes Jul 9, 2018

View reviewed changes

kailuowang merged commit 186dbab into typelevel:master Jul 9, 2018

kailuowang added the enhancement label Jul 23, 2018

		@@ -39,13 +39,13 @@ class KleisliSuite extends CatsSuite {
		checkAll("Semigroupal[Kleisli[Option, Int, ?]]", SerializableTests.serializable(Semigroupal[Kleisli[Option, Int, ?]]))


		type Pair[A] = (A, A)
		checkAll("Pair[String, String] <-> Boolean => String", RepresentableTests[Pair, Boolean].representable[String])


		type Pair[A] = (A, A)

		checkAll("Id[Int] <-> Unit => String", RepresentableTests[Id, Unit].representable[String])

Add Representable Functor #2284

Add Representable Functor #2284

Conversation

eli-jordan commented Jun 7, 2018 • edited Loading

codecov-io commented Jun 7, 2018 • edited Loading

Codecov Report

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-jordan Jun 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-jordan commented Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kailuowang commented Jun 19, 2018

Choose a reason for hiding this comment

eli-jordan commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-jordan Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-jordan commented Jun 23, 2018

LukaJCB commented Jun 23, 2018

eli-jordan commented Jun 23, 2018 • edited Loading

LukaJCB commented Jun 23, 2018

kailuowang commented Jun 25, 2018

kailuowang commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB left a comment

Choose a reason for hiding this comment

eli-jordan commented Jun 7, 2018 •

edited

Loading

codecov-io commented Jun 7, 2018 •

edited

Loading

eli-jordan Jun 8, 2018 •

edited

Loading

eli-jordan commented Jun 12, 2018 •

edited

Loading

eli-jordan Jun 23, 2018 •

edited

Loading

eli-jordan commented Jun 23, 2018 •

edited

Loading