-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add index related helpers to Traverse #1761
Conversation
*/ | ||
def stateTraverse[S, A, B](fa: F[A])(f: A => State[S, B]): State[S, F[B]] = | ||
State[S, F[B]](s => | ||
traverse[State[S, ?] , A, B](fa)(f).run(s).value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need the additional State
wrapping of run(s).value
, right?
This is equal to traverse[State[S, ?] , A, B](fa)(f)
and with SI 2712 fixed, we don't even need to specify the types, so it is just traverse(fa)(f)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To follow up on @peterneyens comment, since this is just equivalent to traverse
, is there any reason to have a separate stateTraverse
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I'm confused why this is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It isn't!
PSA: Don't code while deliriously tired.
Codecov Report
@@ Coverage Diff @@
## master #1761 +/- ##
==========================================
+ Coverage 94.17% 94.21% +0.03%
==========================================
Files 256 256
Lines 4208 4218 +10
Branches 93 97 +4
==========================================
+ Hits 3963 3974 +11
+ Misses 245 244 -1
Continue to review full report at Codecov.
|
e3aa1ff
to
98b6808
Compare
*/ | ||
def stateTraverse[S, A, B](fa: F[A])(f: A => State[S, B]): State[S, F[B]] = | ||
State[S, F[B]](s => | ||
traverse[State[S, ?] , A, B](fa)(f).run(s).value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I'm confused why this is needed.
|
||
test("indexed consistent with zipWithIndex") { | ||
forAll { (fa: List[String]) => | ||
Traverse[List].indexed(fa) should === (fa.zipWithIndex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we try this for a very long list (say 500k elements)? I'm concerned State with traverse can fail for us. I think I have seen issues like that before. I don't want to leave a landmine for someone.
I think we may be able to work around by using tailRecM on State though, but let's see if we need to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a test for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added. Went ahead and created a dedicated file for testing Traverse
instead of piggybacking on the tests in ListTests.scala
.
edaaf4b
to
eef6979
Compare
eef6979
to
50ba29c
Compare
👍 |
seems problematic on scalaJS
|
I did some quick testing. On the JVM, everything is fine. However, on JS the performance of For testing, I used collections from 50,000 to 500,000 entries by increments of 50,000. The tests run reasonably quick for < 100,000 items in the collection. After 300,000 items, the tests get extremely slow or fail. Currently I haven't looked for an underlying problem. For the time being, I have adjusted the tests so that JS is uses a smaller collection. It passed locally-- hopefully it does the same on the build server. |
Do we know that 100,000 isn't enough to test against stack safety for JVM? Now the 3 tests take more than 30 seconds, not sure if that's necessary
|
b0122c9
to
0eee566
Compare
@andyscott @kailuowang Afaik you actually need at most 70000 (see my latest merged PR). |
@edmundnoble Aah! Right now it's been switched to 100000. I'll switch it to 70000 :). |
ase enter the commit message for your changes. Lines starting
0eee566
to
4413058
Compare
Can you also give me something like indexed which takes a function |
something like... // untested
def mapWithIndex[A, B](fa: F[A])(f: (A, Int) => B): F[B] =
traverse(fa)(a => State((s: Int) => (s + 1, f(a, s)))).runA(0).value |
Yeah sure. |
Done. I also updated indexed to be implemented in terms of mapWithIndex. |
8c6e3e6
to
83a0883
Compare
Any objections to also adding def traverseWithIndex[G[_]: Monad, A, B](fa: F[A])(f: (A, Int) => G[B]): G[F[B]] =
traverse(fa)(a => StateT((s: Int) => f(a, s).map(b => (s + 1, b)))).runA(0) |
That's fine with me but I think in the long run it could be better, as soon as we have a |
* The behavior is consistent with the Scala collection library's | ||
* `zipWithIndex` for collections such as `List`. | ||
*/ | ||
def indexed[A](fa: F[A]): F[(A, Int)] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason not to call this zipWithIndex
?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, will do.
} | ||
|
||
test(s"Traverse[$name].mapWithIndex") { | ||
forAll { (fa: F[Int]) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we take ((Int, Int)) => Int
as a function and do:
fa.mapWithIndex((a, i) => fn((a, i))).toList should === (fa.toList.zipWithIndex.map(fn))
|
||
test(s"Traverse[$name].traverseWithIndex") { | ||
forAll { (fa: F[Int]) => | ||
fa.traverseWithIndex((a, i) => Option((a, i))).map(_.toList) should === (Option(fa.toList.zipWithIndex)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we have a stronger test, like take fn: ((Int, Int)) => Option[Int]
and then
val left = fa.traverseWithIndex((a, i) => fn((a, i))).map(_.toList)
val right = (fa.toList.zipWithIndex match {
case Nil => Some(Nil)
case h :: tail => tail.foldM(h :: Nil) { case (l, ai) => fn(ai).map(_ :: l) }.map(_.reverse)
})
left should === right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update incoming. I switched from Option
to Tuple2
to make the stronger test more straightforward.
* 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] = |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing
@johnynek I think I've addressed everything. Using views seemed to be the best way to override Also: I added an "underlying" proxy so I could get coverage of the default method implementations. This also has the very minor benefit of being a really bad benchmark for comparing performance of the default implementation vs the overridden one. I consistently get slightly faster results for the overridden methods:
My benchmarking warmup consists of repeatedly running the one test over and over until the values settle ¯\(ツ)/¯. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Just one minor comment about naming, but I don't have a better suggestion at this moment.
|
||
// 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] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is nice.
* 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]] = |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
…ator for performance
@johnynek I renamed |
merged with 3 sign-offs |
This adds
traverseWithIndex
,mapWithIndex
andzipWithIndex
toTraverse
.traverseWithIndexM
is akin totraverse
, but includes the index of the value in the structureF
. This requires the effect to be monadic.mapWithIndex
is akin tomap
, but includes the index of the value in the structureF
.zipWithIndex
returns F, with values tupled with their indices, just like Scala collection's zipWithIndex.Instances for
List
,Vector
, andStream
have overriddenmapWithIndex
andzipWithIndex
that delegate to related underlying collections methods.mapWithIndex
onList
andVector
uses.iterator
to avoid building an intermediate collection when first zipping with indices.