Foldable/Reducible intercalate update #1522
Conversation
| case one @ NonEmptyList(_, Nil) => one | ||
| case NonEmptyList(hd, tl) => NonEmptyList(hd, a :: intersperseList(tl, a)) | ||
| } | ||
| Reducible[NonEmptyList].reduce(nel) |
There was a problem hiding this comment.
I could probably skip the unnecessary reduce step for one.
toNonEmptyList(fa) match {
case NonEmptyList(hd, Nil) => hd
case NonEmptyList(hd, tl) =>
Reducible[NonEmptyList].reduce(NonEmptyList(hd, a :: intersperseList(tl, a)))
}
|
👍 |
Current coverage is 92.37% (diff: 100%)@@ master #1522 diff @@
==========================================
Files 246 246
Lines 3743 3763 +20
Methods 3622 3637 +15
Messages 0 0
Branches 121 126 +5
==========================================
+ Hits 3459 3476 +17
- Misses 284 287 +3
Partials 0 0
|
peterneyens
force-pushed
the
intercalate-update
branch
from
5aafd1f
to
cc484d6
Compare
|
👍 |
|
Actually it looks like Foldable.combineAll is just an alias for fold which does not call Monoid.combineAll. I suggest we make Foldable.combineAll be Monoid.combineAll(fa.toList) |
peterneyens
force-pushed
the
intercalate-update
branch
from
cc484d6
to
b723dea
Compare
|
Updated to use the It seems we were already using |
|
👍 |
| @@ -82,6 +82,10 @@ trait ListInstances extends cats.kernel.instances.ListInstances { | |||
|
|
|||
| 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) | |||
|
|
|||
| override def fold[A](fa: List[A])(implicit A: Monoid[A]): A = A.combineAll(fa) | |||
There was a problem hiding this comment.
I probably missed something but I am curious what gain we get from overriding this? I mean the
In Monoid.combineAll it's
as.foldLeft(empty)(combine)In Foldable.fold it's
foldLeft(fa, A.empty) { (acc, a) =>
A.combine(acc, a)
}There was a problem hiding this comment.
I was confused about that too.
When you run the MapMonoidBench, you can see that the first using Monoid.combineAll is considerably faster than the second.
That's because Monoid.combineAll is overridden in some instances, eg Map and more general Iterable.
There was a problem hiding this comment.
got it. that's what I suspected, but I only checked the List instance.
|
👍 |
Follow up on #1520, after @johnynek 's comment.
This should reduce the complexity for
intercalatefromO(N^2)toO(N).I also overrode
toListin someFoldableinstances.