Add Traverse1 typeclass (#1577) #1611

LukaJCB · 2017-04-18T13:56:16Z

Resolves #1577. Looking for feedback on this one, not sure if there's an easier way to implement it, or if there's general things to improve.

codecov-io · 2017-04-22T14:10:47Z

Codecov Report

Merging #1611 into master will increase coverage by 0.04%.
The diff coverage is 95.31%.

@@            Coverage Diff             @@
##           master    #1611      +/-   ##
==========================================
+ Coverage   93.88%   93.92%   +0.04%     
==========================================
  Files         246      249       +3     
  Lines        4088     4148      +60     
  Branches      154      155       +1     
==========================================
+ Hits         3838     3896      +58     
- Misses        250      252       +2

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/reducible.scala	`100% <ø> (ø)`	⬆️
core/src/main/scala/cats/Reducible.scala	`94.91% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/package.scala	`100% <100%> (ø)`	⬆️
...in/scala/cats/laws/discipline/ReducibleTests.scala	`100% <100%> (ø)`	⬆️
...a/cats/laws/discipline/NonEmptyTraverseTests.scala	`100% <100%> (ø)`
core/src/main/scala/cats/NonEmptyTraverse.scala	`100% <100%> (ø)`
core/src/main/scala/cats/data/OneAnd.scala	`98.36% <100%> (+0.14%)`	⬆️
core/src/main/scala/cats/data/Nested.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/Composed.scala	`96.15% <100%> (+0.15%)`	⬆️
... 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 1931c93...50cc4c0. Read the comment docs.

edmundnoble

Few comments mostly about testing, but otherwise LGTM.

edmundnoble · 2017-04-29T17:23:04Z

core/src/main/scala/cats/Traverse1.scala

+  def traverse1[G[_]: Apply, A, B](fa: F[A])(f: A => G[B]): G[F[B]]
+
+  def sequence1[G[_]: Apply, A](fga: F[G[A]]): G[F[A]] =
+    traverse1(fga)(identity)


This method is untested.

edmundnoble · 2017-04-29T17:23:53Z

core/src/main/scala/cats/Traverse1.scala

+    traverse1(fga)(identity)
+
+  def flatTraverse1[G[_], A, B](fa: F[A])(f: A => G[F[B]])(implicit G: Apply[G], F: FlatMap[F]): G[F[B]] =
+    G.map(traverse1(fa)(f))(F.flatten)


Ditto (untested) for this, and flatSequence, sequence, traverse1U, sequence1U in the same class.

edmundnoble · 2017-04-29T17:24:11Z

core/src/main/scala/cats/Traverse1.scala

+  def traverse1U[A, GB](fa: F[A])(f: A => GB)(implicit G: Unapply[Apply, GB]): G.M[F[G.A]] =
+    traverse1(fa)(G.subst.compose(f))(G.TC)
+
+  def sequenceU1[GA](fga: F[GA])(implicit U: Unapply[Apply, GA]): U.M[F[U.A]] =


Should this perhaps be sequence1U?

edmundnoble · 2017-04-29T17:25:00Z

core/src/main/scala/cats/Traverse1.scala

+  * This class can be used on any type where the first value (`A`) and
+  * the "rest" of the values (`G[A]`) can be easily found.
+  */
+abstract class NonEmptyTraverse1[F[_], G[_]](implicit G: Foldable[G] with Traverse[G]) extends Traverse1[F] {


edmundnoble · 2017-04-29T17:25:27Z

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

-    new Traverse[OneAnd[F, ?]] {
-      def traverse[G[_], A, B](fa: OneAnd[F, A])(f: (A) => G[B])(implicit G: Applicative[G]): G[OneAnd[F, B]] = {
-        G.map2Eval(f(fa.head), Always(F.traverse(fa.tail)(f)))(OneAnd(_, _)).value
+  implicit def catsDataTraverseForOneAnd[F[_]](implicit F: Traverse[F], F2: MonadCombine[F]): Traverse1[OneAnd[F, ?]] =


I'm curious; why doesn't this use NonEmptyTraverse1?

Hey, I'm not sure what exactly you mean by this comment, if you could clarify, that'd be great :)

@LukaJCB I think that @edmundnoble is asking why the Traverse1 instance for OneAnd isn't using the NonEmptyTraverse1 abstract class that you've submitted with this PR.

edmundnoble · 2017-04-29T17:27:34Z

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

    new NonEmptyReducible[NonEmptyList, List] with SemigroupK[NonEmptyList] with Comonad[NonEmptyList]
-      with Traverse[NonEmptyList] with Monad[NonEmptyList] {
+      with Traverse1[NonEmptyList] with Monad[NonEmptyList] {


This one as well, why not NonEmptyTraverse1 it up?

ceedubs · 2017-05-13T19:03:09Z

Thanks @LukaJCB! This is good stuff.

If you would like to move forward with this, could you please address Edmund's review comments? Also I think that you should be able to implement a compose similar to the one defined for Traverse.

LukaJCB · 2017-05-14T15:19:12Z

Yeah, I'm sorry I've been so inactive, have been quite busy the past few weeks, but I'll get to this in the next couple of days! :)

kailuowang · 2017-05-15T15:06:32Z

core/src/main/scala/cats/Traverse1.scala

+import cats.data.NonEmptyList
+import simulacrum.typeclass
+
+@typeclass trait Traverse1[F[_]] extends Traverse[F] with Reducible[F] {


Can we also add some brief scaladocs there?

kailuowang · 2017-05-15T15:06:57Z

core/src/main/scala/cats/Traverse1.scala

+
+
+
+  def traverse1U[A, GB](fa: F[A])(f: A => GB)(implicit G: Unapply[Apply, GB]): G.M[F[G.A]] =


all unapplied versions are no longer needed

kailuowang · 2017-05-15T15:09:50Z

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

+package syntax
+
+private[syntax] trait Traverse1Syntax1 {
+  implicit def catsSyntaxUTraverse1[FA](fa: FA)(implicit U: Unapply[Traverse1, FA]): Traverse1.Ops[U.M, U.A] =


We don't need this unapply version anymore.

kailuowang · 2017-05-15T15:10:17Z

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

-    new NestedReducibleOps[F, G, A](fga)
+private[syntax] trait ReducibleSyntax1 {
+  implicit def catsSyntaxUReducible[FA](fa: FA)(implicit U: Unapply[Reducible, FA]): Reducible.Ops[U.M, U.A] =
+    new Reducible.Ops[U.M, U.A] {


we can use a doctest in the type class to cover this.

kailuowang · 2017-05-15T15:12:28Z

laws/src/main/scala/cats/laws/Traverse1Laws.scala

+import cats.syntax.traverse1._
+import cats.syntax.reducible._
+
+trait Traverse1Laws[F[_]] extends TraverseLaws[F] with ReducibleLaws[F] {


None of the laws and test were actually hit. We need to add one law test in a data type that has a vanilla Traverse1 instance and one for a data type that uses the NonEmptyTraverse1.

…dable instance

edmundnoble · 2017-05-18T19:57:38Z

core/src/main/scala/cats/Reducible.scala

+ * This class can be used on any type where the first value (`A`) and
+ * the "rest" of the values (`G[A]`) can be easily found.
+ */
+abstract class NonEmptyReducible[F[_], G[_]](implicit G: Foldable[G]) extends Reducible[F] {


implicit *val* G, otherwise this is a param the class is closing over and not a field, thus not serializable.

Unfortunately adding val didn't seem to fix the issue, I'll have to look into it further 🤔

peterneyens

I left a few comments.

The two most important are the one about the traverse1 implementations and about NonEmptyTraverse1.

peterneyens · 2017-05-28T08:48:03Z

core/src/main/scala/cats/Traverse1.scala

+  override def sequence[G[_] : Applicative, A](fga: F[G[A]]): G[F[A]] =
+    traverse1(fga)(identity)
+
+  override def reduceMap[A, B](fa: F[A])(f: (A) => B)(implicit B: Semigroup[B]): B =


I think foldMap1 is implemented in Scalaz using traverse1 because that way Traverse1 implements Foldable1 as well. The primitive operations of Reducible are reduceLeftTo and reduceRightTo.

I don't think it makes any sense for us to override reduceMap here.

Relevant discussion about Traverse implementing Foldable : #107.

peterneyens · 2017-05-28T08:49:07Z

core/src/main/scala/cats/Traverse1.scala

+  override def traverse[G[_] : Applicative, A, B](fa: F[A])(f: (A) => G[B]): G[F[B]] =
+    traverse1(fa)(f)
+
+  override def sequence[G[_] : Applicative, A](fga: F[G[A]]): G[F[A]] =


By overriding traverse we already get this, not sure if we should explicitly override this?

peterneyens · 2017-05-28T08:49:52Z

core/src/main/scala/cats/Traverse1.scala

+  def flatTraverse1[G[_], A, B](fa: F[A])(f: A => G[F[B]])(implicit G: Apply[G], F: FlatMap[F]): G[F[B]] =
+    G.map(traverse1(fa)(f))(F.flatten)
+
+  def flatSequence[G[_], A](fgfa: F[G[F[A]]])(implicit G: Apply[G], F: FlatMap[F]): G[F[A]] =


Did you mean to call this flatSequence1?

peterneyens · 2017-05-28T08:50:26Z

core/src/main/scala/cats/Traverse1.scala

+  override def reduceMap[A, B](fa: F[A])(f: (A) => B)(implicit B: Semigroup[B]): B =
+    reduceLeft(traverse1[Id, A, B](fa)(f))(B.combine)
+
+  override def map[A, B](fa: F[A])(f: A => B): F[B] =


We get this by overriding traverse as well.

peterneyens · 2017-05-28T09:03:24Z

core/src/main/scala/cats/Traverse1.scala

+  * This class can be used on any type where the first value (`A`) and
+  * the "rest" of the values (`G[A]`) can be easily found.
+  */
+abstract class NonEmptyTraverse1[F[_], G[_]](implicit G: Traverse[G]) extends Traverse1[F] {


I am not sure about this. This is just a copy of NonEmptyReducible where the implicit is changed from Foldable[G] to Traverse[G], but we are not actually using anything from Traverse in this class, right?

I would drop this class and use new NonEmptyReducible[F, G] extends Traverse1[G] in place of new NonEmptyTraverse1[F, G].

peterneyens · 2017-05-28T10:43:43Z

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

+      def traverse1[G[_] : Apply, A, B](as: NonEmptyList[A])(f: A => G[B]): G[NonEmptyList[B]] = {
+        as.map(a => Apply[G].map(f(a))(NonEmptyList.of(_)))
+          .reduceLeft((acc, a) => (acc |@| a).map( _ |+| _ ))
+      }


This is not really the best implementation of traverse1.

A better implementation :

def traverse1[G[_], A, B](nel: NonEmptyList[A])(f: A => G[B])(implicit G: Apply[G]): G[NonEmptyList[B]] = Foldable[List].reduceRightToOption[A, G[List[B]]](nel.tail)(a => G.map(f(a))(_ :: Nil)) { (a, lglb) => G.map2Eval(f(a), lglb)(_ :: _) }.map { case None => G.map(f(nel.head))(NonEmptyList(_, Nil)) case Some(gtail) => G.map2(f(nel.head), gtail)(NonEmptyList(_, _)) }.value

Traverse[Vector].traverse also uses prepending (+:), so I think this should work for NonEmptyVector as well. For OneAnd we need to come up with something else if we want to improve that one as well.

A benchmark comparing your traverse1 to mine above and a copy of yours using NonEmptyList(_, Nil) instead of NonEmptyList.of.

[info] Benchmark Mode Cnt Score Error Units [info] NelTraverseBench.traverse1noOf thrpt 10 10977.183 ± 240.703 ops/s [info] NelTraverseBench.traverse1prime thrpt 10 89787.940 ± 849.006 ops/s [info] NelTraverseBench.traverse1 thrpt 10 3929.498 ± 72.846 ops/s

The difference of just replacing NonEmptyList.of is already quite big, so I am plugging #1707 here again.

…raverse1

peterneyens · 2017-05-28T12:39:33Z

Thanks for the quick changes @LukaJCB!
I think you may need to fix the conflicts in the typeclasses.md file before travis runs again.

peterneyens · 2017-05-28T13:56:58Z

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

-      def traverse[G[_], A, B](fa: OneAnd[F, A])(f: (A) => G[B])(implicit G: Applicative[G]): G[OneAnd[F, B]] = {
-        G.map2Eval(f(fa.head), Always(F.traverse(fa.tail)(f)))(OneAnd(_, _)).value
-      }
+  implicit def catsDataTraverse1ForOneAnd[F[_]](implicit F: Traverse[F], F2: MonadCombine[F]): Traverse1[OneAnd[F, ?]] =


Can we override traverse using the implementation originally used in Traverse?
We probably want to override the traverse for NonEmptyVector as well using the previous implementation, come to think of it.

This implementation using MonadCombine also means that you cannot get a Traverse[OneAnd[F, ?]] instance anymore if F has a Traverse but no MonadCombine.

Great idea!

…type

peterneyens · 2017-05-28T21:09:59Z

Sorry to ask you one more thing, but can you add a Traverse1#compose method (to create a Traverse1[λ[α => F[G[α]]]] instance), like @ceedubs asked here?

See:

Traverse#compose
ComposedTraverse
Traverse instance for Nested
Testing compose and Nested instance in NestedTests

We could also merge as is and do this in follow up PR.

LukaJCB · 2017-05-28T21:31:38Z

Don't be! I'm on mobile right now, but I'll do it in the morning tomorrow :)

LukaJCB · 2017-05-29T09:02:23Z

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 {


Not sure if making a new class here was the best thing to do, feedback very welcome :)

I think you can put the catsDataTraverse1ForNested inside NestedInstances.

As you can never have a TraverseFilter and a Traverse1 for a data type at the same time, I don't think that the composition Traverse and TraverseFilter versus the composition of Traverse1 and Traverse1 can be ambiguous.

peterneyens · 2017-05-29T10:43:30Z

Seems that the Reducible[Nested[NonEmptyList, NonEmptyVector, ?]] instance isn't tested any more after we moved the Traverse1 instance.

I think replacing the ReducibleTests[Nested[NonEmptyList, NonEmptyVector, ?]] by

{
  implicit val foldable = ListWrapper.foldable
  checkAll("Nested[NonEmptyList, OneAnd[ListWrapper, ?], ?]", ReducibleTests[Nested[NonEmptyList, OneAnd[ListWrapper, ?], ?]].reducible[Option, Int, Int])
  checkAll("Reducible[Nested[NonEmptyList, OneAnd[ListWrapper, ?], ?]]", SerializableTests.serializable(Reducible[Nested[NonEmptyList, OneAnd[ListWrapper, ?], ?]]))
}

should fix this.

pending rename

kailuowang · 2017-05-30T14:43:35Z

I agree that Traverse1 probably is not a good enough name. I can live with NonEmptyTraverse as suggested by @LukaJCB here

peterneyens · 2017-05-30T16:43:59Z

What do we do in that case with the methodsuffix 1?
Appending NonEmpty becomes a bit unwieldy (traverseNonEmpty, flatSequenceNonEmpty, ...).

LukaJCB · 2017-05-30T19:05:59Z

I think I would prefer traverseNonEmpty, but yeah, your point still holds, nonEmptyFlatSequence is quite a bit. Can't think of anything better right now, but I'll try to brainstorm later.

kailuowang · 2017-06-06T15:26:49Z

I vote for going with NonEmpty, it's not the best, but better than 1

LukaJCB · 2017-06-08T13:24:09Z

We currently also have traverse1_ and sequence1_ in reducible. I suggest renaming them both and since we'd be breaking compatibility anyway, I also suggest we find better names for these two. I realize the underscore comes from the Haskell equivalent, but I do not believe it conveys its intent.

I suggest something along the lines of traverseVoid or traverseToUnit.

Edit: To be consistent this would require changing them in Foldable as well , which might be too much of a breaking change. Could also create another issue for this.

andyscott · 2017-06-08T13:52:10Z

I like NonEmpty as a prefix. It's consistent with the naming for NonEmptyList. Additionally, I prefer nonEmpty as a prefix for the associated methods.

peterneyens · 2017-06-08T14:10:20Z

I don't think we will be able to come up with anything better than NonEmptyTraverse and nonEmptyTraverse / nonEmptySequence, so 👍.

I think it is best to leave the _ suffix in this PR and create a new issue for a potential rename.

kailuowang · 2017-06-08T16:55:15Z

laws/src/main/scala/cats/laws/discipline/NonEmptyTraverseTests.scala

+      def props: Seq[(String, Prop)] = Seq(
+        "nonEmptyTraverse identity" -> forAll(laws.nonEmptyTraverseIdentity[A, C] _),
+        "nonEmptyTraverse sequential composition" -> forAll(laws.traverseSequentialComposition[A, B, C, X, Y] _),
+        "nonEmptyTraverse parallel composition" -> forAll(laws.traverseParallelComposition[A, B, X, Y] _),


should we add tests for laws.nonEmptyTraverseParallelComposition and laws.nonEmptyTraverseSequentialComposition ?

Yeah, not sure what happened there 😅

LukaJCB · 2017-06-12T17:10:52Z

There's currently one last thing, in Reducible we have the intercalate1 method. As with traverse1_ and sequence1_, I'm going to rename this to nonEmptyIntercalate if no one objects.

Luka Jacobowitz and others added 4 commits March 27, 2017 14:36

Add Traverse1 typeclass (typelevel#1577)

5452d01

Add tests and laws for Traverse1 typeclass (typelevel#1577)

96d79d2

Merge branch 'master' into master

4a38545

Format with newline at end of file

3e9afe3

edmundnoble suggested changes Apr 29, 2017

View reviewed changes

edmundnoble reviewed Apr 29, 2017

View reviewed changes

kailuowang suggested changes May 15, 2017

View reviewed changes

Luka Jacobowitz added 5 commits May 18, 2017 11:36

Replace NonEmptyReducible with NonEmptyTraverse1

7ff5e1e

Add Scaladocs to Traverse1

2e2e7ac

Remove unneeded Unapply version

82f4537

Readd Reducible instance to OneAnd so a Monad is not needed for a Fol…

ab40d79

…dable instance

Use NonEmptyTraverse1 for traverse1 implementation of OneAnd

11e8a5d

edmundnoble reviewed May 18, 2017

View reviewed changes

peterneyens requested changes May 28, 2017

View reviewed changes

Luka Jacobowitz added 2 commits May 28, 2017 13:47

Rename flatSequence

6945a26

Remove NonEmptyReducible class and replace with NonEmptyReducible + T…

17d8d62

…raverse1

LukaJCB force-pushed the master branch from 0591bf8 to a8dd218 Compare May 28, 2017 12:29

Replace traverse1 with more performant implementation

b1e115b

LukaJCB force-pushed the master branch from a8dd218 to b1e115b Compare May 28, 2017 12:31

LukaJCB and others added 2 commits May 28, 2017 14:43

Merge branch 'master' into master

796ebb4

Remove Unapply syntax

f437a98

LukaJCB force-pushed the master branch from 167586a to f437a98 Compare May 28, 2017 13:21

peterneyens reviewed May 28, 2017

View reviewed changes

Luka Jacobowitz added 3 commits May 28, 2017 16:29

Separate traverse and traverse1 for OneAnd

647df06

Override traverse implementation of NonEmptyVector

1f69c30

Change type annotation of NonEmptyvector instances to reflect actual …

caac5f7

…type

peterneyens mentioned this pull request May 28, 2017

Multiple things are prefixed withBi- meaning different things. #1297

Closed

Add Traverse1#compose and instance for Nested

f214c64

LukaJCB commented May 29, 2017

View reviewed changes

Move nested traverse1 instance to NestedInstances

9ed10ea

Move reducible nested tests

7f55edc

kailuowang mentioned this pull request May 29, 2017

Prepare for the 1.0.0-MF release #1702

Merged

26 tasks

kailuowang previously approved these changes May 30, 2017

View reviewed changes

kailuowang reviewed Jun 8, 2017

View reviewed changes

rename traverse1 to nonEmptyTraverse

02c25d5

LukaJCB force-pushed the master branch from 1535936 to 02c25d5 Compare June 8, 2017 20:51

kailuowang approved these changes Jun 12, 2017

View reviewed changes

kailuowang added the Source Breaking label Jun 12, 2017

Rename intercalate1 to nonEmptyIntercalate

50cc4c0

LukaJCB mentioned this pull request Jun 12, 2017

Rename traverse_ and sequence_ in Foldable #1723

Closed

kailuowang approved these changes Jun 12, 2017

View reviewed changes

edmundnoble approved these changes Jun 19, 2017

View reviewed changes

kailuowang merged commit afb8a18 into typelevel:master Jun 19, 2017

LukaJCB mentioned this pull request Jun 27, 2017

Add NonEmptyTraverse tut documentation #1747

Closed




		def traverse1U[A, GB](fa: F[A])(f: A => GB)(implicit G: Unapply[Apply, GB]): G.M[F[G.A]] =

Add Traverse1 typeclass (#1577) #1611

Add Traverse1 typeclass (#1577) #1611

Conversation

LukaJCB commented Apr 18, 2017 • edited Loading

codecov-io commented Apr 22, 2017 • edited Loading

Codecov Report

edmundnoble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundnoble Apr 29, 2017 • 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

edmundnoble Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

ceedubs commented May 13, 2017

LukaJCB commented May 14, 2017

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

edmundnoble May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens left a 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

peterneyens May 28, 2017 • edited Loading

Choose a reason for hiding this comment

peterneyens May 28, 2017 • edited Loading

Choose a reason for hiding this comment

peterneyens commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens commented May 28, 2017

LukaJCB commented May 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterneyens commented May 29, 2017

kailuowang commented May 30, 2017

peterneyens commented May 30, 2017

LukaJCB commented May 30, 2017

kailuowang commented Jun 6, 2017

LukaJCB commented Jun 8, 2017 • edited Loading

andyscott commented Jun 8, 2017

peterneyens commented Jun 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Jun 12, 2017

LukaJCB commented Apr 18, 2017 •

edited

Loading

codecov-io commented Apr 22, 2017 •

edited

Loading

edmundnoble Apr 29, 2017 •

edited

Loading

edmundnoble Apr 29, 2017 •

edited

Loading

edmundnoble May 18, 2017 •

edited

Loading

peterneyens May 28, 2017 •

edited

Loading

peterneyens May 28, 2017 •

edited

Loading

LukaJCB commented Jun 8, 2017 •

edited

Loading