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

Jump to bottom

Add NonEmptyList.zipWithIndex #1517

Merged
merged 2 commits into from
Jan 5, 2017
Merged

Add NonEmptyList.zipWithIndex #1517

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3eadf0
Add NonEmptyList.zipWithIndex
cranst0n Jan 5, 2017
3520a74
Use builder instead of recursion for performance reasons.
cranst0n Jan 5, 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
20 changes: 20 additions & 0 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 @@ -165,6 +165,26 @@ final case class NonEmptyList[A](head: A, tail: List[A]) {
go(head, tail, Nil)
}

/**
* Zips each element of this `NonEmptyList` with its index.
*
* {{{
* scala> import cats.data.NonEmptyList
* scala> val nel = NonEmptyList.of("a", "b", "c")
* scala> nel.zipWithIndex
* res0: cats.data.NonEmptyList[(String, Int)] = NonEmptyList((a,0), (b,1), (c,2))
* }}}
*/
def zipWithIndex: NonEmptyList[(A, Int)] = {
@tailrec
def go(rest: List[A], ix: Int, acc: List[(A, Int)]): List[(A, Int)] =
Copy link
Contributor

Choose a reason for hiding this comment

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

while this is a nice implementation, I think ListBuilder can break the rules and mutate Lists and be faster. So, something like;

val bldr = List.newBuilder[(A, Int)]
var idx = 1
val it = tail.iterator
while (it.hasNext) {
  bldr += (it.next, idx)
  idx += 1
}
NonEmptyList((head, 0), bldr.result)

will be faster, which is nice when we are talking about common libraries.

Copy link
Contributor Author

@cranst0n cranst0n Jan 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Sure. I can update with your recommendations. For the record, here's the output of a quick benchmark I ran with the 2 different approaches.

[info] Benchmark                                   (listSize)   Mode  Cnt         Score         Error  Units
[info] NelZipWithIndexBench.zipWithIndexBuilder             1  thrpt   20  81866809.100 ± 1827269.422  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder            10  thrpt   20   8397494.282 ±  265179.762  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder           100  thrpt   20    837125.798 ±   19766.234  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder          1000  thrpt   20     65129.596 ±    1794.893  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder         10000  thrpt   20      6648.203 ±     103.004  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder        100000  thrpt   20       540.067 ±      14.993  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder       1000000  thrpt   20        16.204 ±       5.125  ops/s
[info] NelZipWithIndexBench.zipWithIndexBuilder      10000000  thrpt   20         0.205 ±       0.082  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive           1  thrpt   20  88856828.471 ± 2776243.282  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive          10  thrpt   20   2548195.555 ±   92940.797  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive         100  thrpt   20    244793.012 ±    6162.114  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive        1000  thrpt   20     21696.738 ±     632.243  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive       10000  thrpt   20      4267.619 ±     805.001  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive      100000  thrpt   20       356.116 ±       9.618  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive     1000000  thrpt   20        10.396 ±       2.741  ops/s
[info] NelZipWithIndexBench.zipWithIndexRecursive    10000000  thrpt   20         0.111 ±       0.014  ops/s

Copy link
Contributor

Choose a reason for hiding this comment

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

That's great! Thanks for benchmarking.

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Way to be rigorous! So, if I'm reading this right, if the tail != Nil (listSize > 1) this is ~ 2-3x faster to use the builder. I would have assumed 2x or so because you skip the reverse.

rest match {
case Nil => acc.reverse
case h :: t => go(t, ix + 1, (h, ix) :: acc)
}
NonEmptyList((head, 0), go(tail, 1, Nil))
}

}

object NonEmptyList extends NonEmptyListInstances {
Expand Down
6 changes: 6 additions & 0 deletions tests/src/test/scala/cats/tests/NonEmptyListTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,12 @@ class NonEmptyListTests extends CatsSuite {
nel.reverse.toList should === (nel.toList.reverse)
}
}

test("NonEmptyList#zipWithIndex is consistent with List#zipWithIndex") {
forAll { nel: NonEmptyList[Int] =>
nel.zipWithIndex.toList should === (nel.toList.zipWithIndex)
}
}
}

class ReducibleNonEmptyListCheck extends ReducibleCheck[NonEmptyList]("NonEmptyList") {
Expand Down