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

Add index related helpers to Traverse #1761

Merged
29 changes: 29 additions & 0 deletions core/src/main/scala/cats/Traverse.scala
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
package cats

import cats.data.State
import cats.data.StateT

import simulacrum.typeclass

/**
Expand Down Expand Up @@ -97,4 +100,30 @@ import simulacrum.typeclass

override def map[A, B](fa: F[A])(f: A => B): F[B] =
traverse[Id, A, B](fa)(f)

/**
* Akin to [[map]], but also provides the value's index in structure
* F when calling the function.
*/
def mapWithIndex[A, B](fa: F[A])(f: (A, Int) => B): F[B] =
Copy link
Contributor

Choose a reason for hiding this comment

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

can we override this for List, Vector and Stream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing

traverse(fa)(a =>
State((s: Int) => (s + 1, f(a, s)))).runA(0).value

/**
* Akin to [[traverse]], but also provides the value's index in
* structure F when calling the function.
*/
def traverseWithIndex[G[_], A, B](fa: F[A])(f: (A, Int) => G[B])(implicit G: Monad[G]): G[F[B]] =
Copy link
Contributor

Choose a reason for hiding this comment

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

my only slight reservation with the name here is that traverse takes an Applicative[G] and implies often some parallelism, where this is sequential and requires Monad[G] since the applicative for StateT still requires Monad[G] (I think).

Copy link
Contributor Author

@andyscott andyscott Jul 11, 2017

Choose a reason for hiding this comment

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

I don't really like that I had to require Monad. I suppose this could also be implemented with just Applicative if it's done with two passes on the underlying structure-- traverse(zipWithIndex(f))(ai => fa(ai._1, ai._2)). But I also don't like having to make two passes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Disregard my previous comment that you can't do this. You can use the Group[Int] and WriterT instead of State.

Copy link
Contributor

Choose a reason for hiding this comment

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

if we can relax to Applicative that would be ideal.

Copy link
Contributor Author

@andyscott andyscott Jul 12, 2017

Choose a reason for hiding this comment

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

I can't sort out how to make it work with WriterT. What did you have in mind @edmundnoble?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I can't find a way to do this. I can calculate the length though :P.

traverse(fa)(a =>
StateT((s: Int) => G.map(f(a, s))(b => (s + 1, b)))).runA(0)

/**
* Traverses through the structure F, pairing the values with
* assigned indices.
*
* The behavior is consistent with the Scala collection library's
* `zipWithIndex` for collections such as `List`.
*/
def zipWithIndex[A](fa: F[A]): F[(A, Int)] =
mapWithIndex(fa)((a, i) => (a, i))
}
6 changes: 6 additions & 0 deletions core/src/main/scala/cats/instances/list.scala
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ trait ListInstances extends cats.kernel.instances.ListInstances {
G.map2Eval(f(a), lglb)(_ :: _)
}.value

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

override def zipWithIndex[A](fa: List[A]): List[(A, Int)] =
fa.zipWithIndex

@tailrec
override def get[A](fa: List[A])(idx: Long): Option[A] =
fa match {
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/scala/cats/instances/stream.scala
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,12 @@ trait StreamInstances extends cats.kernel.instances.StreamInstances {
}.value
}

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

override def zipWithIndex[A](fa: Stream[A]): Stream[(A, Int)] =
fa.zipWithIndex

def tailRecM[A, B](a: A)(fn: A => Stream[Either[A, B]]): Stream[B] = {
val it: Iterator[B] = new Iterator[B] {
var stack: Stream[Either[A, B]] = fn(a)
Expand Down
6 changes: 6 additions & 0 deletions core/src/main/scala/cats/instances/vector.scala
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ trait VectorInstances extends cats.kernel.instances.VectorInstances {
G.map2Eval(f(a), lgvb)(_ +: _)
}.value

override def mapWithIndex[A, B](fa: Vector[A])(f: (A, Int) => B): Vector[B] =
fa.view.zipWithIndex.map(ai => f(ai._1, ai._2)).toVector
Copy link
Contributor

Choose a reason for hiding this comment

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

can we benchmark with fa.iterator.zipWithIndex.map(..., I think .iterator may be faster than .view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An official benchmark in the benchmark submodule? Or will a quick local benchmark work, updating the code according?


override def zipWithIndex[A](fa: Vector[A]): Vector[(A, Int)] =
fa.zipWithIndex

override def exists[A](fa: Vector[A])(p: A => Boolean): Boolean =
fa.exists(p)

Expand Down
76 changes: 76 additions & 0 deletions tests/src/test/scala/cats/tests/TraverseTests.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
package cats
package tests

import org.scalatest.prop.PropertyChecks
import org.scalacheck.Arbitrary

import cats.instances.all._

abstract class TraverseCheck[F[_]: Traverse](name: String)(implicit ArbFInt: Arbitrary[F[Int]]) extends CatsSuite with PropertyChecks {

test(s"Traverse[$name].zipWithIndex") {
forAll { (fa: F[Int]) =>
fa.zipWithIndex.toList should === (fa.toList.zipWithIndex)
}
}

test(s"Traverse[$name].mapWithIndex") {
forAll { (fa: F[Int], fn: ((Int, Int)) => Int) =>
fa.mapWithIndex((a, i) => fn((a, i))).toList should === (fa.toList.zipWithIndex.map(fn))
}
}

test(s"Traverse[$name].traverseWithIndex") {
forAll { (fa: F[Int], fn: ((Int, Int)) => (Int, Int)) =>
val left = fa.traverseWithIndex((a, i) => fn((a, i))).map(_.toList)
val (xs, values) = fa.toList.zipWithIndex.map(fn).unzip
left should === ((xs.combineAll, values))
}
}

}

object TraverseCheck {
// forces testing of the underlying implementation (avoids overridden methods)
abstract class Underlying[F[_]: Traverse](name: String)(implicit ArbFInt: Arbitrary[F[Int]])
extends TraverseCheck(s"$name (underlying)")(proxyTraverse[F], ArbFInt)

// proxies a traverse instance so we can test default implementations
// to achieve coverage using default datatype instances
private def proxyTraverse[F[_]: Traverse]: Traverse[F] = new Traverse[F] {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is nice.

def foldLeft[A, B](fa: F[A], b: B)(f: (B, A) => B): B =
Traverse[F].foldLeft(fa, b)(f)
def foldRight[A, B](fa: F[A], lb: cats.Eval[B])(f: (A, Eval[B]) => Eval[B]): Eval[B] =
Traverse[F].foldRight(fa, lb)(f)
def traverse[G[_]: Applicative, A, B](fa: F[A])(f: A => G[B]): G[F[B]] =
Traverse[F].traverse(fa)(f)
}
}

class TraverseListCheck extends TraverseCheck[List]("List")
class TraverseStreamCheck extends TraverseCheck[Stream]("Stream")
class TraverseVectorCheck extends TraverseCheck[Vector]("Vector")

class TraverseListCheckUnderlying extends TraverseCheck.Underlying[List]("List")
class TraverseStreamCheckUnderlying extends TraverseCheck.Underlying[Stream]("Stream")
class TraverseVectorCheckUnderlying extends TraverseCheck.Underlying[Vector]("Vector")

class TraverseTestsAdditional extends CatsSuite {

def checkZipWithIndexedStackSafety[F[_]](fromRange: Range => F[Int])(implicit F: Traverse[F]): Unit = {
F.zipWithIndex(fromRange(1 to 70000))
()
}

test("Traverse[List].zipWithIndex stack safety") {
checkZipWithIndexedStackSafety[List](_.toList)
}

test("Traverse[Stream].zipWithIndex stack safety") {
checkZipWithIndexedStackSafety[Stream](_.toStream)
}

test("Traverse[Vector].zipWithIndex stack safety") {
checkZipWithIndexedStackSafety[Vector](_.toVector)
}
}