Issue 3141 #3150

Twizty · 2019-11-14T12:01:35Z

No description provided.

travisbrown

I think these additions need more motivation—it seems like passing Applicative.monoid or Apply.semigroup explicitly as needed isn't that bad.

travisbrown · 2019-11-14T12:47:55Z

core/src/main/scala/cats/Foldable.scala

+    * res0: Right[Int] = Right(3)
+    * }}}
+    */
+  def foldA[G[_], A](fga: F[G[A]])(implicit G: Applicative[G], A: Monoid[A]): G[A] =


I'm not sure about the name here, and I don't think we want to set a precedent for adding methods that just replace a A: Monoid argument with a G[A] + G: Applicative[G], A: Monoid argument, or at least not without a clear case that doing so makes some common usage much more convenient.

travisbrown · 2019-11-14T12:48:41Z

core/src/main/scala/cats/Foldable.scala

+    * }}}
+    */
+  def foldA[G[_], A](fga: F[G[A]])(implicit G: Applicative[G], A: Monoid[A]): G[A] =
+    foldLeft(fga, G.pure(A.empty)) { (acc, ga) =>


Is there a reason to prefer this implementation over fold(fga)(G.monoid)?

This is better, I didn't know about this function.

travisbrown · 2019-11-14T12:52:03Z

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

+    * Reduce a `F[G[A]]` value using `Applicative[G]` and `Semigroup[A]`, a universal
+    * semigroup for `G[_]`.
+    *
+    * This method is a generalization of `reduce`.


I'm not sure about "generalization" here—in what sense is the reduce signature less general?

Also, couldn't the constraint both here and below be Apply instead of Applicative?

travisbrown · 2019-11-14T12:53:35Z

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

+    * This method is a generalization of `reduce`.
+    */
+  def reduceA[G[_], A](fga: F[G[A]])(implicit G: Applicative[G], A: Semigroup[A]): G[A] =
+    reduceLeft(fga)((ga1, ga2) => G.map2(ga1, ga2)(A.combine))


Why not reduce(fga)(Apply[G].semigroup)?

travisbrown · 2019-11-14T13:03:36Z

Oh, just saw the reference to #3141. I think @LukaJCB's idea there is to add A equivalents to M methods—so e.g. foldA would be the applicative implementation of foldM, not fold.

codecov-io · 2019-11-15T15:39:03Z

Codecov Report

Merging #3150 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3150      +/-   ##
==========================================
+ Coverage   93.04%   93.04%   +<.01%     
==========================================
  Files         376      376              
  Lines        7374     7379       +5     
  Branches      209      208       -1     
==========================================
+ Hits         6861     6866       +5     
  Misses        513      513

Flag	Coverage Δ
#scala_version_212	`93.38% <100%> (ø)`	⬆️
#scala_version_213	`90.66% <100%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
core/src/main/scala/cats/Reducible.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/Foldable.scala	`98.5% <100%> (+0.01%)`	⬆️
core/src/main/scala/cats/syntax/foldable.scala	`84.61% <100%> (+0.61%)`	⬆️
core/src/main/scala/cats/syntax/reducible.scala	`100% <100%> (ø)`	⬆️

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 8867e97...c63b8bc. Read the comment docs.

travisbrown

@Twizty Did you see my comment above? These methods don't have the right signatures for #3141. Take reduceMapM:

def reduceMapM[G[_], A, B](fa: F[A])(f: A => G[B])(implicit G: FlatMap[G], B: Semigroup[B]): G[B]

The applicative version of this would be something like:

def reduceMapA[G[_], A, B](fa: F[A])(f: A => G[B])(implicit G: Apply[G], B: Semigroup[B]): G[B] =
  reduceRightTo(fa)(f)((a, egb) => G.map2Eval(f(a), egb)(B.combine)).value

Not:

def reduceMapA[G[_], A, B](fga: F[G[A]])(f: A => B)(implicit G: Apply[G], B: Semigroup[B]): G[B]

I'm not entirely sure what @LukaJCB had in mind for foldA and reduceA—I'm guessing something like this for foldA, on the analogy of foldMapA in #3130 or my reduceMapA above:

def foldA[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Applicative[G]): G[B]

But at a glance I don't think it's possible to implement this. In any case the version you have doesn't really seem like it should be called foldA—it's more like a less generic combineAll.

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

tests/src/test/scala-2.12/cats/tests/NonEmptyStreamSuite.scala

Twizty · 2019-11-18T13:03:33Z

@travisbrown Yes, I've seen that comment, and I've forgotten to reply. I tried to implement def foldA[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Applicative[G]): G[B], but it didn't seem possible, I've asked @LukaJCB, and he said that he expected def foldA[G[_]: Applicative, A: Monoid](fga: F[G[A]]): G[A] and something similar for reduceA. I've implemented reduceMapA similar to reduceA, and foldA, even though it is possible to implement it with like you did or even without using Eval. I can rename foldA and reduceA, and add def reduceMapA[G[_], A, B](fa: F[A])(f: A => G[B])(implicit G: Apply[G], B: Semigroup[B]): G[B]. Would you like me to do that?

travisbrown · 2019-11-18T13:16:07Z

@Twizty @LukaJCB My understanding was that #3141 is about taking methods with a Monad constraint and providing applicative versions (which will sometimes be less performant because they can't rely on tailRecM). This is what foldMapA is in #3130.

I don't think it's a good idea to have a foldM and foldA that do completely different things and have completely different signatures, and I'm not really sure when you'd ever need the F[G[A]] version of foldA provided here, given that we have fold / combineAll. Can you clarify what you were intending, @LukaJCB?

LukaJCB · 2019-11-18T13:42:06Z

I was thinking of foldA as an Applicative version of fold AKA combineAll. We already have precedent with MonoidK, foldK and foldMapK.
I figured these identies would hold:

fa.foldMap(identity) === fa.fold
fa.foldMapK(identity) === fa.foldK
fa.reduceMap(identity) === fa.reduce
fa.reduceMapK(identity) === fa.reduceK

// this one is the outlier
fa.foldMapM(identity) === fa.foldM

I assumed foldM would be the same, but turns out it's more like a foldLeftM, which is unfortunate. Maybe we should deprecate it?

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

Twizty · 2019-11-19T11:33:23Z

@LukaJCB Is there anything else I need to do on this issue?

travisbrown · 2019-11-19T12:19:04Z

Sorry but please don't merge this yet—I still have a couple of issues…

Twizty · 2019-11-19T12:20:38Z

@travisbrown is there anything I need to do?

travisbrown

Sorry, got distracted by the bincompat breakage fixes in #3162 and #3163 yesterday.

There are two things I think still need to be addressed for reduceMapA:

The first is that the implementation you've given of reduceMapA here won't short-circuit even if reduceRightTo is sufficiently lazy, while the implementation I've given in a comment above will short-circuit appropriately. I just noticed that this is also the case for reduceMapM, which I think is a bug. Both foldMapM and foldMapA short-circuit when the underlying operations support it, and it seems like both reduceMap versions should do the same. We can fix reduceMapM in a separate PR, but I don't think we should introduce a broken reduceMapA here.
If we're introducing a reduceMapA with almost exactly the same signature and exactly the same semantics as reduceMapM, I think we need to explain the apparent redundancy in the API docs, similar to what we did in add foldMapA as an alternative to foldMapM that only requires an Applicative rather than a monad #3130.

I have to admit I also still just totally don't understand why we need foldA or reduceA, or how e.g. a foldA with the signature here is "an applicative version of fold".

We currently have the following:

def fold[A](fa: F[A])(implicit A: Monoid[A]): A
def foldM[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B]

And this PR adds:

def foldA[G[_], A](fga: F[G[A]])(implicit G: Applicative[G], A: Monoid[A]): G[A]

I think there are two problems with this. This first is that by analogy with foldMapA/M, etc., I'd expect foldA to look like this:

def foldA[G[_], A, B](fa: F[A], z: B)(f: (B, A) => G[B])(implicit G: Applicative[G]): G[B]

But it's not possible to implement this in general for Foldable. I think that should rule out having a method named foldA at all because of the potential confusion, but even assuming we were okay with having a foldA, I'm just not sure what potential use cases there are for this method:

def foldA[G[_], A](fga: F[G[A]])(implicit G: Applicative[G], A: Monoid[A]): G[A]

It feels entirely redundant with fold. If we have a Applicative[G] and a Monoid[A], then in every case I can think of we'll also have a Monoid[G[A]], and we can just use fold. Even if we don't have a Monoid[G[A]], we can trivially construct one with Applicative.monoid[G, A].

travisbrown · 2019-11-20T14:19:34Z

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

@@ -29,5 +29,9 @@ final class ReducibleOps0[F[_], A](private val fa: F[A]) extends AnyVal {
   * a: String = "foo321"
   * }}}
   * */
-  def reduceMapK[G[_], B](f: A => G[B])(implicit F: Reducible[F], G: SemigroupK[G]): G[B] = F.reduceMapK[G, A, B](fa)(f)
+  def reduceMapK[G[_], B](f: A => G[B])(implicit F: Reducible[F], G: SemigroupK[G]): G[B] =
+    F.reduceLeftTo(fa)(f)((b, a) => G.combineK(b, f(a)))


Why did you change this? In general we want to avoid having implementation logic in syntax type classes.

Yes, my bad, I'll remove this diff.

travisbrown · 2019-11-20T15:40:58Z

I've addressed the reduceMapM issue here: #3167.

Twizty · 2019-11-20T16:45:47Z

@travisbrown I've tested your implementation and it also doesn't short-circuit. In my test "Reducible[$name].reduceMapA failure case" I added a log into intToString function:

def intToString(i: Long): Either[String, Int] = {
    println(("!!!", i))
    if (i == 2) i.toInt.asRight else "boom!!!".asLeft
}

It prints all the numbers from 1 to 3 for NonEmptyList and NonEmptyVector. foldMapA does short-circuit appropriately. I will fix the short-curcuit bug in my implementation.

travisbrown · 2019-11-20T16:53:15Z

@Twizty It depends on whether reduceRightTo short-circuits, and some of the current standard library instances don't.

travisbrown · 2019-11-20T16:57:46Z

@Twizty The idea is that default implementations in the type class shouldn't undo the short-circuiting behavior of the abstract type class operation—i.e. if foldRight short-circuits then foldMapA/M should as well, and if reduceRightTo short-circuits then reduceMapA/M should.

Twizty · 2019-11-20T18:12:31Z

@travisbrown that's what I thought. Anyway do I need to do anything or is it better to just close this pull request?

travisbrown · 2019-11-21T12:33:53Z

@Twizty I was thinking about this more last night and I was wrong: there is actually a reasonable use case for a method with the foldA signature here, but not with either your original implementation or my proposed simplification. 😄

Suppose we want to sum up numbers in a list as long as they're all positive, but want to fail if we see zero or a negative number, and we want to log the positive numbers we successfully added:

import cats.Foldable, cats.data.{EitherT, Writer}, cats.implicits._

type W[A] = Writer[List[Int], A]
def checkPos(x: Int): EitherT[W, Int, Int] =
  if (x > 0) EitherT.right(Writer(List(x), x)) else EitherT.leftT(x)

Then we can do something like this:

scala> List(1, 2, 3, 4).foldMapA(checkPos)
res0: cats.data.EitherT[W,Int,Int] = EitherT(WriterT((List(1, 2, 3, 4),Right(10))))

scala> List(1, 2, 3, 4, 0, 5).foldMapA(checkPos)
res1: cats.data.EitherT[W,Int,Int] = EitherT(WriterT((List(1, 2, 3, 4),Left(0))))

Or:

scala> Foldable[List].fold(List(1, 2, 3, 4).map(checkPos))
res4: cats.data.EitherT[W,Int,Int] = EitherT(WriterT((List(1, 2, 3, 4),Right(10))))

scala> Foldable[List].fold(List(1, 2, 3, 4, 0, 5).map(checkPos))
res5: cats.data.EitherT[W,Int,Int] = EitherT(WriterT((List(1, 2, 3, 4, 5),Left(0))))

But we might want the short-circuiting behavior of foldMapA for the case where we've already mapped, like this:

scala> Foldable[List].foldA(List(1, 2, 3, 4, 0, 5).map(checkPos))
res6: cats.data.EitherT[W,Int,Int] = EitherT(WriterT((List(1, 2, 3, 4),Left(0))))

We might also want this to be lazy:

scala> import cats.Foldable, cats.implicits._
import cats.Foldable
import cats.implicits._

scala> def checkPos(x: Int): Either[Int, Int] = if (x > 0) Right(x) else Left(x)
checkPos: (x: Int)Either[Int,Int]

scala> val s = (Stream(1, 2, 3, 0) ++ Stream.from(5))
s: scala.collection.immutable.Stream[Int] = Stream(1, ?)

scala> Foldable[Stream].foldA(s.map(checkPos))
res0: Either[Int,Int] = Left(0)

In both cases the behavior is different from fold, which can't either be lazy or short-circuit. Neither of our implementations supports both of these, but the following does:

def foldA[G[_], A](fga: F[G[A]])(implicit G: Applicative[G], A: Monoid[A]): G[A] =
  foldMapA(fga)(identity)

So I'm convinced now we should have this method, and even that we could probably call it foldA, if we also deprecate foldM, which seems to be a mistake we got into by copy-pasting Haskell's Control.Monad. 😄

We also definitely need to explain all of this clearly in the API docs.

Again, sorry to drag out this PR, but it's turned up a really interesting set of questions and problems with the current state of this stuff—thanks for helping to push this forward.

Twizty · 2019-11-21T18:29:29Z

@travisbrown thank you for such a detailed response! I will fix my implementation then. Do we need reduceA and reduceMapA?

travisbrown

I'm working on a follow-up PR right now, but I think we can go ahead and merge this one. @LukaJCB?

Twizty added 3 commits November 14, 2019 14:58

typelevel#3141 [WIP] Implemented .foldA

2ce81ea

typelevel#3141 [WIP] Implemented .reduceA

ac9efdc

typelevel#3141 [WIP] Implemented .reduceMapA

6d28b9c

travisbrown reviewed Nov 14, 2019

View reviewed changes

Twizty added 4 commits November 15, 2019 16:44

typelevel#3141 Refactored

f914fb5

typelevel#3141 Added tests

aa36227

typelevel#3141 Fixed 2.13

ae5a3d7

typelevel#3141 Added link to the issue at simulacrum

1ff4848

Twizty changed the title ~~[WIP] Issue 3141~~ Issue 3141 Nov 15, 2019

typelevel#3141 Fixed docs

07f8d38

Twizty added 2 commits November 18, 2019 11:35

Merge branch 'master' into issue3141

971f380

typelevel#3141 Formatted

d59858a

travisbrown suggested changes Nov 18, 2019

View reviewed changes

core/src/main/scala/cats/syntax/foldable.scala Show resolved Hide resolved

tests/src/test/scala-2.12/cats/tests/NonEmptyStreamSuite.scala Show resolved Hide resolved

typelevel#3141 Refactored

619b1b4

typelevel#3141 Changed .reduceMap signature

c910cea

LukaJCB reviewed Nov 18, 2019

View reviewed changes

core/src/main/scala/cats/Reducible.scala Outdated Show resolved Hide resolved

Twizty added 2 commits November 19, 2019 11:41

typelevel#3141 Removed noop on .reduceMapA

82c16e1

typelevel#3141 Fixed docs

6eebfeb

LukaJCB previously approved these changes Nov 19, 2019

View reviewed changes

travisbrown suggested changes Nov 20, 2019

View reviewed changes

typelevel#3141 Removed changes from reduceMapK

c63b8bc

Twizty dismissed LukaJCB’s stale review via c63b8bc November 20, 2019 15:21

travisbrown mentioned this pull request Nov 20, 2019

Make default reduceMapM lazy if reduceRightTo is lazy #3167

Merged

travisbrown approved these changes Dec 10, 2019

View reviewed changes

LukaJCB approved these changes Dec 10, 2019

View reviewed changes

LukaJCB merged commit 70c9625 into typelevel:master Dec 10, 2019

This was referenced Dec 10, 2019

Rename rangeE for clarity #3198

Merged

Fix foldA, reduceA, and reduceMapA short-circuiting #3199

Merged

Optimize or remove reduceMapM? #3200

Open

travisbrown added this to the 2.1.0-RC3 milestone Dec 10, 2019

travisbrown added the enhancement label Dec 10, 2019

travisbrown mentioned this pull request Dec 18, 2019

Add foldA, reduceMapA and reduceA #3141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 3141 #3150

Issue 3141 #3150

Twizty commented Nov 14, 2019

travisbrown left a comment

travisbrown Nov 14, 2019

travisbrown Nov 14, 2019

Twizty Nov 14, 2019

travisbrown Nov 14, 2019

travisbrown Nov 14, 2019

travisbrown commented Nov 14, 2019

codecov-io commented Nov 15, 2019 •

edited

Loading

travisbrown left a comment

Twizty commented Nov 18, 2019

travisbrown commented Nov 18, 2019 •

edited

Loading

LukaJCB commented Nov 18, 2019

Twizty commented Nov 19, 2019

travisbrown commented Nov 19, 2019

Twizty commented Nov 19, 2019

travisbrown left a comment •

edited

Loading

travisbrown Nov 20, 2019

Twizty Nov 20, 2019

travisbrown commented Nov 20, 2019

Twizty commented Nov 20, 2019

travisbrown commented Nov 20, 2019

travisbrown commented Nov 20, 2019

Twizty commented Nov 20, 2019

travisbrown commented Nov 21, 2019 •

edited

Loading

Twizty commented Nov 21, 2019

travisbrown left a comment

Issue 3141 #3150

Issue 3141 #3150

Conversation

Twizty commented Nov 14, 2019

travisbrown left a comment

Choose a reason for hiding this comment

travisbrown Nov 14, 2019

Choose a reason for hiding this comment

travisbrown Nov 14, 2019

Choose a reason for hiding this comment

Twizty Nov 14, 2019

Choose a reason for hiding this comment

travisbrown Nov 14, 2019

Choose a reason for hiding this comment

travisbrown Nov 14, 2019

Choose a reason for hiding this comment

travisbrown commented Nov 14, 2019

codecov-io commented Nov 15, 2019 • edited Loading

Codecov Report

travisbrown left a comment

Choose a reason for hiding this comment

Twizty commented Nov 18, 2019

travisbrown commented Nov 18, 2019 • edited Loading

LukaJCB commented Nov 18, 2019

Twizty commented Nov 19, 2019

travisbrown commented Nov 19, 2019

Twizty commented Nov 19, 2019

travisbrown left a comment • edited Loading

Choose a reason for hiding this comment

travisbrown Nov 20, 2019

Choose a reason for hiding this comment

Twizty Nov 20, 2019

Choose a reason for hiding this comment

travisbrown commented Nov 20, 2019

Twizty commented Nov 20, 2019

travisbrown commented Nov 20, 2019

travisbrown commented Nov 20, 2019

Twizty commented Nov 20, 2019

travisbrown commented Nov 21, 2019 • edited Loading

Twizty commented Nov 21, 2019

travisbrown left a comment

Choose a reason for hiding this comment

codecov-io commented Nov 15, 2019 •

edited

Loading

travisbrown commented Nov 18, 2019 •

edited

Loading

travisbrown left a comment •

edited

Loading

travisbrown commented Nov 21, 2019 •

edited

Loading