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

Adds foldM #356

Closed
wants to merge 3 commits into from
Closed

Adds foldM #356

wants to merge 3 commits into from

Conversation

eed3si9n
Copy link
Contributor

This implements foldM.

val names = List("Aaron", "Betty", "Calvin", "Deirdra")
val sumM = F.foldM(names, "") { (acc, x) => (Some(acc + x): Option[String]) }
assert(sumM == Some("AaronBettyCalvinDeirdra"))
val notCalvin = F.foldM(names, "") { (acc, x) => 
  if (x == "Calvin") (None: Option[String])
  else (Some(acc + x): Option[String]) }
assert(notCalvin == None)

@@ -26,6 +26,12 @@ class FoldableTests extends CatsSuite {
// more basic checks
val names = List("Aaron", "Betty", "Calvin", "Deirdra")
assert(F.foldMap(names)(_.length) == names.map(_.length).sum)
val sumM = F.foldM(names, "") { (acc, x) => (Some(acc + x): Option[String]) }
assert(sumM == Some("AaronBettyCalvinDeirdra"))
val notCalvin = F.foldM(names, "") { (acc, x) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Scalastyle doesn't like some trailing whitespace here.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 21, 2015

This looks good to me once the whitespace is fixed and the build is passing.

@non could you please be a second set of eyes on the use of partialFold/Fold/Lazy, since you are the most familiar with that?

@eed3si9n
Copy link
Contributor Author

Added the whitespace fix.

It's based on foldRight but I wasn't able to get it to not blow the stack, so I made it eager.

@codecov-io
Copy link

Current coverage is 59.41%

Merging #356 into master will change coverage by +3.30% by d086843

Coverage Diff

@@            master    #356   diff @@
======================================
  Files          136     132     -4
  Stmts         1818    1599   -219
  Branches        25      25       
  Methods          0       0       
======================================
- Hit           1020     950    -70
  Partial          0       0       
+ Missed         798     649   -149

Powered by Codecov

@non
Copy link
Contributor

non commented Jun 22, 2015

This looks good. I want to see if I can make it stack-safe, but in principle I'm 👍 on this.

@eed3si9n
Copy link
Contributor Author

I suspect that to make it completely lazy, it's not enough to fold lazily, but also make G.flatMap part lazy, which involves trampolining it?

@ceedubs
Copy link
Contributor

ceedubs commented Jul 9, 2015

I'm a little hesitant on this one, now that I know it isn't stack-safe. If we think we could make it stack-safe in the future, I'm less concerned. Could we at least leave a scaladoc comment warning about stack safety?

@eed3si9n
Copy link
Contributor Author

eed3si9n commented Jul 9, 2015

I think the issue was that I could make it lazy, but laziness only goes to calculating a flatMap of a flatMap of a flatMap etc, so when you put the value into it it blows the stack. So to fix that we need another datatype beyond Lazy like Trampoline?

@ceedubs
Copy link
Contributor

ceedubs commented Jul 13, 2015

@eed3si9n yes, I suspect that you are right.

@milessabin
Copy link
Member

s/Trampoline/Eval/

@adelbertc
Copy link
Contributor

This seems useful but it looks like it got lost in the PRs - very sorry @eed3si9n . If looks like there's some actual failing tests: https://travis-ci.org/typelevel/cats/builds/99910227 Once that's fixed this LGTM

@TomasMikula
Copy link
Contributor

You seem to first build a function, that you then apply at once to the argument. Unless the target monad G is lazy (such as Free), the function you build is not stack-safe. To see this, let's view this piece of code

(k: B => G[B]) =>
        w: B => G.flatMap(f(w, a))(k)

as

ki := w => G.flatMap(f(w, ai))(ki-1)

Thus, if G.flatMap is eager, then kn invokes kn-1 invokes kn-2 invokes ... invokes k0.

Now, for the solution, I might be missing something, but wouldn't an eager foldLeft work?

@DavidGregory084
Copy link
Member

@adelbertc I'm fairly certain this was submitted when FoldableTestsAdditional was flickering (see #769); it's possible that if you close and reopen the PR it will pass.

@eed3si9n
Copy link
Contributor Author

Rebased against master to prod Travis.

@eed3si9n
Copy link
Contributor Author

Here's how to blow the stack:

scala> import cats._, std.all._, syntax.all._
import cats._
import std.all._
import syntax.all._

scala> def nonzero(acc: Int, x: Int): Option[Int] =
     |   if (x == 0) None else Some(acc + x)
nonzero: (acc: Int, x: Int)Option[Int]

scala> (Foldable[List].foldM((1 to 10000).toList, Eval.later { 0 }) {nonzero}).value
java.lang.StackOverflowError
  at $anonfun$2.apply(<console>:21)
  at $anonfun$2.apply(<console>:21)
  at cats.Foldable$$anonfun$foldM$2$$anonfun$apply$5$$anonfun$apply$6.apply(Foldable.scala:86)
  at scala.Option.flatMap(Option.scala:171)
  at cats.std.OptionInstances$$anon$1.flatMap(option.scala:20)
  at cats.std.OptionInstances$$anon$1.flatMap(option.scala:8)
  at cats.Foldable$$anonfun$foldM$2$$anonfun$apply$5$$anonfun$apply$6.apply(Foldable.scala:86)
  at scala.Option.flatMap(Option.scala:171)
  at cats.std.OptionInstances$$anon$1.flatMap(option.scala:20)
  at cats.std.OptionInstances$$anon$1.flatMap(option.scala:8)
  at cats.Foldable$$anonfun$foldM$2$$anonfun$apply$5$$anonfun$apply$6.apply(Foldable.scala:86)
  at scala.Option.flatMap(Option.scala:171)

@TomasMikula TomasMikula mentioned this pull request Mar 10, 2016
@eed3si9n
Copy link
Contributor Author

Closing this in favor of #925

@eed3si9n eed3si9n closed this Mar 10, 2016
@ceedubs ceedubs mentioned this pull request May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants