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.

Sign up for GitHub

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

remove iteratorFoldM fixes #1716 #1740

Merged
merged 8 commits into from
Jun 26, 2017
Merged

remove iteratorFoldM fixes #1716 #1740

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
33352a2
remove iteratorFoldM fixes #1716
kailuowang Jun 23, 2017
ba99572
fix foldM to not evaluate one extra uneccessary element
kailuowang Jun 23, 2017
9705b47
update comment
kailuowang Jun 23, 2017
e2f94dc
more efficient implementations
kailuowang Jun 23, 2017
e955595
keep the original sequence
kailuowang Jun 23, 2017
a8b54a9
updated NonEmptyVector as well
kailuowang Jun 23, 2017
e2f53ae
use default foldM implemenation to test short-circuit
kailuowang Jun 23, 2017
312ee35
style correction / remove redundant overrides
kailuowang Jun 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 0 additions & 35 deletions core/src/main/scala/cats/Foldable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,41 +414,6 @@ object Foldable {
loop()
}

/**
* Implementation of [[Foldable.foldM]] which can short-circuit for
* structures with an `Iterator`.
*
* For example we can sum a `Stream` of integers and stop if
* the sum reaches 100 (if we reach the end of the `Stream`
* before getting to 100 we return the total sum) :
*
* {{{
* scala> import cats.implicits._
* scala> type LongOr[A] = Either[Long, A]
* scala> def sumStream(s: Stream[Int]): Long =
* | Foldable.iteratorFoldM[LongOr, Int, Long](s.toIterator, 0L){ (acc, n) =>
* | val sum = acc + n
* | if (sum < 100L) Right(sum) else Left(sum)
* | }.merge
*
* scala> sumStream(Stream.continually(1))
* res0: Long = 100
*
* scala> sumStream(Stream(1,2,3,4))
* res1: Long = 10
* }}}
*
* Note that `Foldable[Stream].foldM` uses this method underneath, so
* you wouldn't call this method explicitly like in the example above.
*/
def iteratorFoldM[M[_], A, B](it: Iterator[A], z: B)(f: (B, A) => M[B])(implicit M: Monad[M]): M[B] = {
val go: B => M[Either[B, B]] = { b =>
if (it.hasNext) M.map(f(b, it.next))(Left(_))
else M.pure(Right(b))
}
M.tailRecM(z)(go)
}


/**
* Isomorphic to
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/data/NonEmptyList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,9 +431,6 @@ private[data] sealed trait NonEmptyListInstances extends NonEmptyListInstances0
override def fold[A](fa: NonEmptyList[A])(implicit A: Monoid[A]): A =
fa.reduce

override def foldM[G[_], A, B](fa: NonEmptyList[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.toList.toIterator, z)(f)

override def find[A](fa: NonEmptyList[A])(f: A => Boolean): Option[A] =
fa find f

Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/data/NonEmptyVector.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,9 +264,6 @@ private[data] sealed trait NonEmptyVectorInstances {
override def fold[A](fa: NonEmptyVector[A])(implicit A: Monoid[A]): A =
fa.reduce

override def foldM[G[_], A, B](fa: NonEmptyVector[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.toVector.toIterator, z)(f)

override def find[A](fa: NonEmptyVector[A])(f: A => Boolean): Option[A] =
fa.find(f)

Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/instances/list.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,6 @@ trait ListInstances extends cats.kernel.instances.ListInstances {

override def filter[A](fa: List[A])(f: A => Boolean): List[A] = fa.filter(f)

override def foldM[G[_], A, B](fa: List[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.toIterator, z)(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we maybe do this one?

def step(in: (List[A], B])): G[Either[(List[A], B), A]] = in match {
  case (Nil, b) => G.pure(Right(b))
  case (a :: tail, b) => G.map(f(a, b)) { bnext => Left((tail, bnext)) }
} 

G.tailRecM((fa, z))(step)

right? If we had this, we could cover the common case for List and NonEmptyList, no?


override def fold[A](fa: List[A])(implicit A: Monoid[A]): A = A.combineAll(fa)

override def toList[A](fa: List[A]): List[A] = fa
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/instances/map.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,6 @@ trait MapInstances extends cats.kernel.instances.MapInstances {

override def isEmpty[A](fa: Map[K, A]): Boolean = fa.isEmpty

override def foldM[G[_], A, B](fa: Map[K, A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.valuesIterator, z)(f)

override def fold[A](fa: Map[K, A])(implicit A: Monoid[A]): A =
A.combineAll(fa.values)

Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/instances/set.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,6 @@ trait SetInstances extends cats.kernel.instances.SetInstances {

override def isEmpty[A](fa: Set[A]): Boolean = fa.isEmpty

override def foldM[G[_], A, B](fa: Set[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.toIterator, z)(f)

override def fold[A](fa: Set[A])(implicit A: Monoid[A]): A = A.combineAll(fa)

override def toList[A](fa: Set[A]): List[A] = fa.toList
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/instances/stream.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,6 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances {

override def collect[A, B](fa: Stream[A])(f: PartialFunction[A, B]): Stream[B] = fa.collect(f)

override def foldM[G[_], A, B](fa: Stream[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.toIterator, z)(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

we could copy the List approach above here:

def step(in: (Stream[A], B])): G[Either[(Stream[A], B), A]] = in match {
  case (Stream.empty, b) => G.pure(Right(b))
  case (a #:: tail, b) => G.map(f(a, b)) { bnext => Left((tail, bnext)) }
} 

G.tailRecM((fa, z))(step)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Patten match Stream into a #:: tail will force an evaluation of the head of the tail, so I used vanilla if


override def fold[A](fa: Stream[A])(implicit A: Monoid[A]): A = A.combineAll(fa)

override def toList[A](fa: Stream[A]): List[A] = fa.toList
Expand Down
3 changes: 0 additions & 3 deletions core/src/main/scala/cats/instances/vector.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,6 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances {

override def collect[A, B](fa: Vector[A])(f: PartialFunction[A, B]): Vector[B] = fa.collect(f)

override def foldM[G[_], A, B](fa: Vector[A], z: B)(f: (B, A) => G[B])(implicit G: Monad[G]): G[B] =
Foldable.iteratorFoldM(fa.toIterator, z)(f)
Copy link
Contributor

Choose a reason for hiding this comment

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

Vector.tail is so slow, we are probably better off doing fa.toList.foldM here if we do the above.


override def fold[A](fa: Vector[A])(implicit A: Monoid[A]): A = A.combineAll(fa)

override def toList[A](fa: Vector[A]): List[A] = fa.toList
Expand Down
19 changes: 10 additions & 9 deletions tests/src/test/scala/cats/tests/FoldableTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,7 @@ class FoldableTestsAdditional extends CatsSuite {
}
assert(result.value)

// test trampolining
val large = Stream((1 to 10000): _*)
assert(contains(large, 10000).value)

// test laziness of foldM
dangerous.foldM(0)((acc, a) => if (a < 2) Some(acc + a) else None) should === (None)
}

test(".foldLeftM short-circuiting") {
Expand All @@ -214,17 +209,23 @@ class FoldableTestsAdditional extends CatsSuite {
}

test(".foldLeftM short-circuiting optimality") {
// test that no more elements are evaluated than absolutely necessary
// test that no more than 1 elements are evaluated than absolutely necessary
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we look if we can solve this additional evaluation with @TomasMikula's suggestion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

Copy link
Collaborator

@peterneyens peterneyens Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

You forgot to update this comment. Fixed it already 👍


def concatUntil(ss: Stream[String], stop: String): Either[String, String] =
Foldable[Stream].foldLeftM[Either[String, ?], String, String](ss, "") { (acc, s) =>
if (s == stop) Left(acc) else Right(acc + s)
}

def boom: Stream[String] = sys.error("boom")
assert(concatUntil("STOP" #:: boom, "STOP") == Left(""))
assert(concatUntil("Zero" #:: "STOP" #:: boom, "STOP") == Left("Zero"))
assert(concatUntil("Zero" #:: "One" #:: "STOP" #:: boom, "STOP") == Left("ZeroOne"))
assert(concatUntil("STOP" #:: "AFTER" #:: boom, "STOP") == Left(""))
assert(concatUntil("Zero" #:: "STOP" #:: "AFTER" #:: boom, "STOP") == Left("Zero"))
assert(concatUntil("Zero" #:: "One" #:: "STOP" #:: "AFTER" #:: boom, "STOP") == Left("ZeroOne"))
}

test("Foldable[List] doesn't break substitution") {
val result = List.range(0,10).foldM(List.empty[Int])((accum, elt) => Eval.always(elt :: accum))

assert(result.value == result.value)
}
}

Expand Down