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

Cofree comonad #1446

Merged
merged 1 commit into from
Dec 21, 2016
Merged

Cofree comonad #1446

merged 1 commit into from
Dec 21, 2016

Conversation

edmundnoble
Copy link
Contributor

Basic Cofree Comonad functionality, including a couple of instances. I tried to implement a few other things (mapBranching(F ~> G)(Cofree[F, A]): Cofree[G, A], decompile(F ~> W)(W[A]): Cofree[F, A]) but I couldn't make them stack-safe. This implementation is inspired by the one in Scalaz.

@codecov-io
Copy link

codecov-io commented Oct 28, 2016

Current coverage is 92.02% (diff: 100%)

Merging #1446 into master will increase coverage by 0.08%

@@             master      #1446   diff @@
==========================================
  Files           244        245     +1   
  Lines          3623       3660    +37   
  Methods        3499       3539    +40   
  Messages          0          0          
  Branches        124        121     -3   
==========================================
+ Hits           3331       3368    +37   
  Misses          292        292          
  Partials          0          0          

Powered by Codecov. Last update c9521e1...52067df

* A tree with data at the branches, as opposed to Free which is a tree with data at the leaves.
* Not an instruction set functor made into a program monad as in Free, but an instruction set's outputs as a
* functor made into a tree of the possible worlds reachable using the instruction set.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this derived/based on Scalaz's Cofree or from scratch? If former could you add attribution to Scalaz, similar to what we have here ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally derived from the one at Scalaz. I did not know there was an example of such an attribution somewhere in the code to crib from, I'll add it in.


checkAll("Cofree[Option, ?]", ComonadTests[Cofree[Option, ?]].comonad[Int, Int, Int])
checkAll("Cofree[Option, ?]", FoldableTests[Cofree[Option, ?]].foldable[Int, Int])
checkAll("Cofree[Option, ?]", TraverseTests[Cofree[Option, ?]].traverse[Int, Int, Int, Int, Option, Option])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Traverse checks Foldable already

Copy link
Contributor

Choose a reason for hiding this comment

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

Check Serializability of the Traverse instance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't need to check that the Traverse and Foldable instances are valid separately? They are defined separately. I will add that serializable check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely right. Took the foldable tests out.

checkAll("Comonad[Cofree[Option, ?]]", SerializableTests.serializable(Comonad[Cofree[Option, ?]]))
checkAll("Cofree[List, ?]", ComonadTests[Cofree[List, ?]].comonad[Int, Int, Int])
checkAll("Cofree[List, ?]", FoldableTests[Cofree[List, ?]].foldable[Int, Int])
checkAll("Cofree[List, ?]", TraverseTests[Cofree[List, ?]].traverse[Int, Int, Int, Int, Option, Option])
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to test these given the tests above? Seems the only difference is s/Option/List

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're comfy with not testing these, absolutely. I don't see any precedent in other tests in doing the same law checks on different instances, so I'll go ahead and remove.

@edmundnoble edmundnoble force-pushed the cofree-comonad-2 branch 3 times, most recently from df17df2 to 4a4db93 Compare October 29, 2016 01:34
@edmundnoble
Copy link
Contributor Author

Is this alright for inclusion or should it perhaps have more operators?

@adelbertc
Copy link
Contributor

Could you write some functions for the other functions, maybe like unfold and unfoldStrategy ?

Also looks like the Foldable-specific instance isn't being tested because the test is getting the Foldable through the Traverse test. You can see an example here of how we test these kinds of "if F[_]: TC then MT[F, ?]: TC law checks.

Other than that, LGTM 👍 Thanks @edmundnoble !

}

sealed abstract class CofreeInstances2 {
/** low priority `Foldable1` instance */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a Foldable, not Foldable1. There is scalaz.Foldable1. but cats does not have Foldable1.

#145

Copy link
Contributor

Choose a reason for hiding this comment

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

@xuwei-k We have Foldable1 but we call it Reducible #298. No Traverse1 though like you said below.

}

sealed abstract class CofreeInstances1 extends CofreeInstances2 {
/** low priority `Traverse1` instance */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a Traverse, not Traverse1

@edmundnoble edmundnoble force-pushed the cofree-comonad-2 branch 2 times, most recently from 0db086d to 85f6852 Compare November 4, 2016 02:59
@edmundnoble
Copy link
Contributor Author

I removed unfoldStrategy because after trying to write a test, I'm not convinced it has any use. If anybody can see one, please suggest it. Otherwise, I believe I have addressed all of your comments.

@dwhitney
Copy link

dwhitney commented Dec 2, 2016

Bump. This looks interesting and it looks like @edmundnoble has addressed @adelbertc's concerns. Any reason not to merge?


/** Evaluate just the tail. */
def forceTail: Cofree[S, A] =
Cofree[S, A](head, Eval.now(tailEval.value))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not tested, can we add a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeppers. They'll use side effects but meh, I'll steal them from the eval tests.


/** Evaluate the entire Cofree tree. */
def forceAll(implicit S: Functor[S]): Cofree[S, A] =
Cofree[S, A](head, Eval.now(tailEval.map(S.map(_)(_.forceAll)).value))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not tested, can we add a test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absotively.

override final def reduceLeftTo[A, B](fa: Cofree[F, A])(z: A => B)(f: (B, A) => B): B =
F.foldLeft(fa.tailForced, z(fa.head))((b, cof) => foldLeft(cof, b)(f))

override def reduceRightTo[A, B](fa: Cofree[F, A])(z: A => B)(f: (A, Eval[B]) => Eval[B]): Eval[B] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you do this vs something like:

f(ta.head, fa.tailEval.map { reduceRightTo(_)(z)(f) })

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try doing that yourself. Doesn't typecheck as far as I can see; you're expecting tailEval to hold an Eval[Cofree[F, A]] instead of an Eval[F[Cofree[F, A]]].

implicit val iso = CartesianTests.Isomorphisms.invariant[Cofree[Option, ?]]

checkAll("Cofree[Option, ?]", ComonadTests[Cofree[Option, ?]].comonad[Int, Int, Int])
locally {
Copy link
Contributor

Choose a reason for hiding this comment

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

woah what's this locally thing do?

Copy link
Contributor

Choose a reason for hiding this comment

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

@edmundnoble
Copy link
Contributor Author

I've added tests for the methods @johnynek pointed out and fixed the tests for mapBranchingS/mapBranchingT/mapBranchingRoot/unfold which weren't running (but looked to be).


}

sealed abstract class CofreeInstances2 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe you could make the CofreeInstancesX classes and the CofreeComonad, CofreeReducible and CofreeTraversable traits package private (private[free]) (mostly so they don't show up in the ScalaDoc) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Very right. One more feature to add to the eventual pedantic-scala tool that will come out once scala.meta has a semantic API ;)

@edmundnoble edmundnoble force-pushed the cofree-comonad-2 branch 3 times, most recently from 7dd8e06 to 56fc0c4 Compare December 11, 2016 09:46
@adelbertc
Copy link
Contributor

LGTM 👍

}

sealed private[free] abstract class CofreeInstances1 extends CofreeInstances2 {
/** low priority `Traverse` instance */
Copy link
Contributor

@xuwei-k xuwei-k Dec 12, 2016

Choose a reason for hiding this comment

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

I wrote the comments in scalaz.Cofree because there are two methos in scalaz.

  /** low priority `Traverse1` instance */
  implicit def cofreeTraverse[F[_]: Traverse]: Traverse1[Cofree[F, ?]] =
  /** high priority `Traverse1` instance. more efficient */
  implicit def cofreeTraverse1[F[_]: Traverse1]: Traverse1[Cofree[F, ?]] =

Cats have only one method for Traverse[Cofree[F, ?]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh I was wondering why. Removed.

@adelbertc
Copy link
Contributor

@xuwei-k Looks like @edmundnoble removed it - what do you think?

Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

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

👍 from me.

* [[https://github.com/scalaz/scalaz/blob/series/7.3.x/core/src/main/scala/scalaz/Cofree.scala Scalaz's Cofree]],
* originally written by Rúnar Bjarnason.
*/
final case class Cofree[S[_], A](head: A, tailEval: Eval[S[Cofree[S, A]]]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just call this tail? In general it seems like the convention is to add the suffix only when it's needed for disambiguation (although afaik that's not documented anywhere).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The suffix is there to disambiguate it with tailForced, but I think it's reasonable for tail to be the safe version.

package free

/**
* A free comonad for some branching functor `S`. Branching is done lazily using Eval.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is super nitpicky but could you put [[]] around Eval and the Free in the next line? When I'm reading Scaladocs I always get pissed off if the authors make me search to follow a reference to another class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeppers.

/** Cofree anamorphism, lazily evaluated. */
def unfold[F[_], A](a: A)(f: A => F[A])(implicit F: Functor[F]): Cofree[F, A] =
Cofree[F, A](a, Eval.later(F.map(f(a))(unfold(_)(f))))

Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have the monadic ana as well.

def unfoldM[M[_]: Monad, F[_]: Traverse, A](a: A)(f: A => M[F[A]]): M[Cofree[F, A]]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :)

/**
* A stack-safe algebraic recursive fold out of the cofree comonad.
*/
def cata[F[_], A, B](cof: Cofree[F, A])(folder: (A, F[B]) => Eval[B])(implicit F: Traverse[F]): Eval[B] =
Copy link
Member

Choose a reason for hiding this comment

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

I am unhappy that Eval forces Traverse[F] when the strict version would only need Functor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let the record show that I am also made unhappy by this.

Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

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

I had a couple comments but this looks good to me. 👍

@edmundnoble
Copy link
Contributor Author

All cleaned up @tpolecat and @travisbrown, thanks for the reviews :)

Cofree[F, A](a, Eval.later(F.map(f(a))(unfold(_)(f))))

/** Cofree monadic anamorphism, lazily evaluated. */
def unfoldM[F[_], M[_], A](a: A)(f: A => M[F[A]])(implicit F: Traverse[F], M: Monad[M]): M[Cofree[F, A]] = {
Copy link
Member

@tpolecat tpolecat Dec 21, 2016

Choose a reason for hiding this comment

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

Can you convince me this is lazy? I don't quite see it.

Copy link
Member

Choose a reason for hiding this comment

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

(Corrected: I meant to say lazy.)

Copy link
Contributor Author

@edmundnoble edmundnoble Dec 21, 2016

Choose a reason for hiding this comment

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

I cannot for the life of me figure out how to make it lazy. I have removed it for now just because it's unsafe otherwise, but I'd like to add it back soon. Edit: Maybe for CofreeT. Edit two: I believe the reason for this is we want to get all the effects on top of the type, but we want to keep the values inside the type. Can't bind if you haven't done the work yet.

Copy link
Member

Choose a reason for hiding this comment

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

FINE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:O

Removed Cofree[List, A] tests and added attribution

Remove unused imports

Add Reducible instance, separate tests, fix Traverse1 comment

Removed unfoldStrategy, added a test for unfold

Tests for Cofree.mapBranchingRoot/mapBranchingS/T

Add Cofree.tailForced and Cofree.forceAll tests, fix notrunning tests

Make instances classes package-private

Added Cofree.cata/cataM

Add docs

Add test for forceTail

Remove comments

tailEval=>tail, bracket doc references

Add unfoldM

Removed unfoldM
@dwhitney
Copy link

dwhitney commented Dec 21, 2016 via email

@edmundnoble
Copy link
Contributor Author

@dwhitney I tried running it and it looped forever eagerly. It's correct in that it produces the right answer, but it's incorrect in that it makes Cofree's laziness useless.

@peterneyens
Copy link
Collaborator

Thanks @edmundnoble !

@peterneyens peterneyens merged commit d49985d into typelevel:master Dec 21, 2016
@kailuowang kailuowang mentioned this pull request Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants