override more mapAccumulate methods in Traverse #4214

johnynek · 2022-05-28T21:39:17Z

follow up to #4209 to override I think all the remaining values for mapAccumulate.

Also did a few minor changes I noticed along the way.

johnynek · 2022-05-28T21:39:26Z

johnynek · 2022-05-28T21:39:50Z

alleycats-core/src/main/scala/alleycats/std/iterable.scala

@@ -65,6 +65,18 @@ trait IterableInstances {
      override def traverse[G[_], A, B](fa: Iterable[A])(f: A => G[B])(implicit G: Applicative[G]): G[Iterable[B]] =
        if (fa.isEmpty) G.pure(Iterable.empty)
        else G.map(Chain.traverseViaChain(toImIndexedSeq(fa))(f))(_.toVector)
+
+      override def mapAccumulate[S, A, B](init: S, fa: Iterable[A])(f: (S, A) => (S, B)): (S, Iterable[B]) = {


we can't use the StaticMethod implementation here because we aren't in the cats package.

There is also no guarantee that the Iterable would be strict.

good point.

johnynek · 2022-05-28T21:40:08Z

alleycats-core/src/main/scala/alleycats/std/map.scala

      override def map[A, B](fa: Map[K, A])(f: A => B): Map[K, B] =
        fa.map { case (k, a) => (k, f(a)) }

      def foldLeft[A, B](fa: Map[K, A], b: B)(f: (B, A) => B): B =
-        fa.foldLeft(b) { case (x, (k, a)) => f(x, a) }
+        fa.foldLeft(b) { case (x, (_, a)) => f(x, a) }


clear a warning I saw.

johnynek · 2022-05-28T21:40:24Z

alleycats-core/src/main/scala/alleycats/std/set.scala

@@ -120,6 +120,18 @@ trait SetInstances {
        }.value
      }

+      override def mapAccumulate[S, A, B](init: S, fa: Set[A])(f: (S, A) => (S, B)): (S, Set[B]) = {


can't use StaticMethods version outside of cats package.

johnynek · 2022-05-28T21:41:04Z

core/src/main/scala-2.13+/cats/data/NonEmptyLazyList.scala

@@ -213,7 +213,7 @@ class NonEmptyLazyListOps[A](private val value: NonEmptyLazyList[A])
   * Tests if some element is contained in this NonEmptyLazyList
   */
  final def contains(a: A)(implicit A: Eq[A]): Boolean =
-    toLazyList.contains(a)
+    toLazyList.exists(A.eqv(_, a))


this looks like a straight bug. The contains method was ignoring the A. This is technically changing behavior, but I would argue it is a bug fix.

johnynek · 2022-05-28T21:41:24Z

core/src/main/scala-2.13+/cats/instances/arraySeq.scala

-        loop(0).value
-      }
+      def traverse[G[_], A, B](fa: ArraySeq[A])(f: A => G[B])(implicit G: Applicative[G]): G[ArraySeq[B]] =
+        G.map(Chain.traverseViaChain(fa)(f))(_.iterator.to(ArraySeq.untagged))


make traverse stack safe on ArraySeq. We missed this before.

This one is eluding me a bit... How come the previous implementation was not stack safe if each its call to loop is suspended in Eval? Just curious, because I even tried this snippet:

val arr1 = ArraySeq.iterate(1, 100)(_ + 1) val arr2 = arr1.traverse { a => println(s"\n>>> a = $a") new Exception().printStackTrace(Console.out) a.some }

And seems that stack depth is kept as O(1) for the entire traverse evaluation.
Or am I missing something?

Does that work when you make 100 something like 100000?

The fn call you are printing is at constant depth but there is more to it, there is also the mapping function on the map2.

That said, we have hit many issues with traverse stack safety in the past and I think the approach used here was the same there (but maybe I am wrong).

Does that work when you make 100 something like 100000?

Yes, it does even for 1,000,000 :)

The fn call you are printing is at constant depth but there is more to it, there is also the mapping function on the map2.

That said, we have hit many issues with traverse stack safety in the past and I think the approach used here was the same there (but maybe I am wrong).

I mean I'm not against the new implementation (perhaps it's supposed to be more performant). Just curious, because I tried out and didn't manage to screw up the old one :)

Try with Klesli:

#3960

#3947

That's the issue the motivated the previous changes.

Maybe the real problem was that Kleisli has a bad map2Eval, but it seems you can fix it by avoiding a linear depth in map2Eval.

johnynek · 2022-05-28T21:41:46Z

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

@@ -225,7 +225,12 @@ sealed abstract class Ior[+A, +B] extends Product with Serializable {
   * res2: Option[Int] = Some(123)
   * }}}
   */
-  final def right: Option[B] = fold(_ => None, b => Some(b), (_, b) => Some(b))
+  final def right: Option[B] =
+    this match {


optimization to avoid lambdas when pattern matching will do.

johnynek · 2022-05-28T21:42:04Z

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

      def foldLeft[B, C](fa: A Ior B, b: C)(f: (C, B) => C): C =
        fa.foldLeft(b)(f)
      def foldRight[B, C](fa: A Ior B, lc: Eval[C])(f: (B, Eval[C]) => Eval[C]): Eval[C] =
        fa.foldRight(lc)(f)

-      override def size[B](fa: A Ior B): Long = fa.fold(_ => 0L, _ => 1L, (_, _) => 1L)
+      override def size[B](fa: A Ior B): Long =


optimization to avoid lambdas when pattern matching will do.

How about simply if (fa.isLeft) 0L else 1L.

Oh, yikes, isLeft and friends need to be optimized too (maybe by dynamic dispatch ~~which is how Option is implemented~~ nope, was wrong about Option).

Regardless, I still think this method would be better written in terms of isLeft.

BalmungSan · 2022-05-28T21:44:54Z

alleycats-core/src/main/scala/alleycats/std/iterable.scala

@@ -65,6 +65,18 @@ trait IterableInstances {
      override def traverse[G[_], A, B](fa: Iterable[A])(f: A => G[B])(implicit G: Applicative[G]): G[Iterable[B]] =
        if (fa.isEmpty) G.pure(Iterable.empty)
        else G.map(Chain.traverseViaChain(toImIndexedSeq(fa))(f))(_.toVector)
+
+      override def mapAccumulate[S, A, B](init: S, fa: Iterable[A])(f: (S, A) => (S, B)): (S, Iterable[B]) = {


What about also adding mapWithIndex and zipWithIndex here?

override def mapWithIndex[A, B](fa: Iterable[A])(f: (A, Int) => B): Iterable[B] = fa.zipWithIndex.map(ai => f(ai._1, ai._2)) override def zipWithIndex[A](fa: Iterable[A]): Iterable[(A, Int)] = fa.zipWithIndex

The motivation being they would preserve their underling class.

I made this change but I'm suspect about the trying to add invariants we don't really promise (returning the same underlying type).

this is already in alleycats where there are reasons to believe things are suspect. In this case Iterable is so weak it doesn't necessarily iterate in any meaningful order (otherwise it would be Seq).

when people start to assume we offer some stronger contracts, they often wind up getting into trouble and file issues and then we are in a jam. For instance, mapWithIndex is exercising a different code path from mapAccumulate here, and if they want some other law we don't promise (like maybe they do a cast at the end because they assume the Iterable is of the same type), then we get into weird issues of explaining why we went out of our way to maintain the class, but sometimes fail to.

anyway, it's alleycats, so I added it. But I hope people won't rely on it.

johnynek · 2022-05-31T22:23:53Z

@armanbilge can you review this?

danicheg

LGTM.

danicheg · 2022-06-02T15:02:35Z

alleycats-core/src/main/scala/alleycats/std/map.scala

+      override def mapAccumulate[S, A, B](init: S, fa: Map[K, A])(f: (S, A) => (S, B)): (S, Map[K, B]) = {
+        val iter = fa.iterator
+        var s = init
+        val m = Map.newBuilder[K, B]


What do you think about adding the call of m.sizeHint? I haven't a strong opinion it's required, considering extra computing of Map length, but someone used it in cats.

good call. I'll update that.

armanbilge

Thanks, LGTM!

override more mapAccumulate methods in Traverse

906256f

johnynek commented May 28, 2022

View reviewed changes

BalmungSan previously approved these changes May 28, 2022

View reviewed changes

add some iterable overrides

002d4c3

johnynek dismissed BalmungSan’s stale review via 002d4c3 May 28, 2022 22:24

armanbilge added this to the 2.8.0 milestone May 29, 2022

add Id instances

7c1706b

armanbilge self-requested a review May 31, 2022 22:24

danicheg previously approved these changes Jun 2, 2022

View reviewed changes

johnynek added 2 commits June 4, 2022 12:56

Merge branch 'main' into oscar/20220528_more_mapAccumulate

dbfa2bf

review comments

205e7f8

johnynek dismissed danicheg’s stale review via 205e7f8 June 4, 2022 23:04

armanbilge approved these changes Jun 4, 2022

View reviewed changes

johnynek merged commit 1346a0e into main Jun 4, 2022

armanbilge deleted the oscar/20220528_more_mapAccumulate branch June 4, 2022 23:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

override more mapAccumulate methods in Traverse #4214

override more mapAccumulate methods in Traverse #4214

johnynek commented May 28, 2022

johnynek commented May 28, 2022

johnynek May 28, 2022

BalmungSan May 28, 2022

johnynek May 28, 2022

johnynek May 28, 2022

johnynek May 28, 2022

johnynek May 28, 2022

johnynek May 28, 2022

satorg May 31, 2022

johnynek May 31, 2022

satorg May 31, 2022

johnynek May 31, 2022 •

edited

Loading

johnynek May 28, 2022

johnynek May 28, 2022

armanbilge Jun 4, 2022

armanbilge Jun 4, 2022 •

edited

Loading

johnynek Jun 4, 2022

BalmungSan May 28, 2022

johnynek May 28, 2022

johnynek commented May 31, 2022

danicheg left a comment

danicheg Jun 2, 2022

johnynek Jun 3, 2022

johnynek Jun 4, 2022

armanbilge left a comment

override more mapAccumulate methods in Traverse #4214

override more mapAccumulate methods in Traverse #4214

Conversation

johnynek commented May 28, 2022

johnynek commented May 28, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek May 31, 2022 • 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

armanbilge Jun 4, 2022 • 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

johnynek commented May 31, 2022

danicheg 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

armanbilge left a comment

Choose a reason for hiding this comment

johnynek May 31, 2022 •

edited

Loading

armanbilge Jun 4, 2022 •

edited

Loading