Add Chain #2371
Add Chain #2371
Conversation
| } | ||
|
|
||
| /** Applies the supplied function to each element, left to right. */ | ||
| private final def foreach(f: A => Unit): Unit = { |
There was a problem hiding this comment.
I made this method private, though I think we could potentially just rename it to unsafeForeach instead? WDYT?
|
A checkbox for |
|
I'm not super clear how the whole lincensing business works, but as far as I can tell, both fs2 and cats are under MIT, so the code shouldn't have to be relicensed, correct? |
Codecov Report
@@ Coverage Diff @@
## master #2371 +/- ##
==========================================
- Coverage 95.01% 94.84% -0.17%
==========================================
Files 349 350 +1
Lines 6000 6249 +249
Branches 227 281 +54
==========================================
+ Hits 5701 5927 +226
- Misses 299 322 +23
Continue to review full report at Codecov.
|
|
I think |
|
ah sorry. this is no effects. My bad :-) |
|
@pchlupacek I just mentioned you, because you three showed up in the git history of |
|
On another note, I added some of the benchmarks that used to be on fs2 as well as adding some other new ones, and I gotta say again, using |
|
ah thanks, so happy to contribute then :-) |
|
@LukaJCB I want to benchmark this against chain (https://github.com/non/chain), but I'm +1 to having something like this available. It's a great optimization. |
|
also note @non's chain has .iterator: https://github.com/non/chain/blob/master/src/main/scala/chain/Chain.scala#L42 Also, there are some optimizations around accepting batches (which may or may not be a win, but I think the benchmarks were pretty good). |
|
@non Chain and Catenable are remarkably similar. I wonder if |
|
My 2 cents on licensing: change COPYING.md from copyright holder of Erik to "Cats Contributors" and ensure Pavel and Paul are added to AUTHORS.md. I think that's sufficient (IANAL). |
|
As usual I have no clue wrt licensing. I'm ok with Michael's suggestion. |
|
I support @mpilquist's suggestion of changing the copyright to Cats Contributors. I'd just check first with Pavel and Paul to be sure they are OK being added to AUTHORS.md (in the past I think there have been folks that did not want to be listed there). |
|
I'd be happy to benchmark against |
|
Initial benchmarks for accumulating using |
|
Full benchmarks including Also implemented an |
|
Whatever is faster. |
|
Shall we merge this one, then? :) |
| foldLeft(nil: Chain[B])((acc, a) => acc ++ f(a)) | ||
|
|
||
| /** Applies the supplied function to each element and returns a new Chain from the concatenated results */ | ||
| final def flatMapIterator[B](f: A => Chain[B]): Chain[B] = { |
There was a problem hiding this comment.
I assume these iterator variants were just temporary for benchmarking?
Thanks for doing these. Interesting to see which ones were faster.
There was a problem hiding this comment.
Do we want two flatMaps? Can we just keep the faster one?
There was a problem hiding this comment.
Yeah, woops, that's on me, I did just want to keep the faster ones 😄
| result | ||
| } | ||
|
|
||
| final def findIterator(f: A => Boolean): Option[A] = { |
There was a problem hiding this comment.
can we just keep the faster find?
| m | ||
| } | ||
|
|
||
| final def groupByIterator[B](f: A => B)(implicit B: Order[B]): SortedMap[B, Chain[A]] = { |
There was a problem hiding this comment.
let's just keep the faster one.
|
@kailuowang @mpilquist @non Any additional comments or are we ready to merge? :) |
|
|
||
| implicit def catsDataEqForChain[A](implicit A: Eq[A]): Eq[Chain[A]] = new Eq[Chain[A]] { | ||
| def eqv(x: Chain[A], y: Chain[A]): Boolean = | ||
| (x eq y) || x.toList === y.toList |
There was a problem hiding this comment.
might be worthwhile to optimize the second part as x.iterator.sameElements(y.iterator)
There was a problem hiding this comment.
that would ignore Eq[A] no?
I would just copy and paste the four here:
https://github.com/typelevel/cats/blob/master/kernel/src/main/scala/cats/kernel/instances/list.scala#L28
(Eq, Order, Hash, PartialOrder) and use similar matching. We can follow up, especially if we write the implicits not that just convert to list, then make a PR to fix them to be fast.
There was a problem hiding this comment.
I think I got a fairly fast implementation now using iterator :)
|
Going to merge now :) |
|
🎉 Thanks for doing this! So.... when can we get this in a released version? I'd really like to have it for FS2 1.0, which is scheduled for early September. |
|
Thanks for all the work Luka! |
| if (rights.isEmpty) { | ||
| c = null | ||
| } else { | ||
| c = rights.last |
There was a problem hiding this comment.
This branch is not being tested which worries me. Can we think of a way to exercise this part?
|
|
||
| /** Creates a Chain from the specified elements. */ | ||
| def apply[A](as: A*): Chain[A] = | ||
| as match { |
There was a problem hiding this comment.
Wouldn’t it be better to just do fromSeq(as) here?
I notice currently the second branch of the case match is untested.
There was a problem hiding this comment.
Chain(1, 2, 3, 4) will exercise the WrappedArray case. Now that we have a Wrap constructor, it's probably better to replace this specialization with fromSeq.
| case 1 => A.arbitrary.map(Chain.one) | ||
| case 2 => A.arbitrary.flatMap(a1 => A.arbitrary.flatMap(a2 => | ||
| Chain.concat(Chain.one(a1), Chain.one(a2)))) | ||
| case n => Chain.fromSeq(Range.apply(0, n)).foldLeft(Gen.const(Chain.empty[A])) { (gen, _) => |
There was a problem hiding this comment.
I don’t know what the fromSeq is doing here because we are just folding it. I don’t love this generator because it does not generate any internal Wrap nodes and is always left associated.
Could we do something like: for size n we either wrap a seq or create a two random chains of size a and b such that a + b == n and we combine those.
If we do this, I think we can get the coverage up nice and high.
|
@mpilquist okay, let's start preparing for a 1.3 release. |
|
Can we address my comments? This PR did decrease our overall coverage and has a few untested branches. |
|
I can make a PR to address these. @LukaJCB has done plenty already. |
|
Sorry, I guess I merged too soon, I can also address these 👍 |
* Add Catenable * Add law tests and simple arbitrary and eq instances * More tests * Add benchmarks * Add chain benchmarks * Add Iterator * More chain benchmarks * Add Paul and Pavel to Authors and change COPYING to Cats Contributors * More Tests * Add reverse, groupBy and zipWith * Add Collection Wrapping optimization * Add traverse and foldRight implementations that don't convert to List * Rename to Chain; add reverseIterator * More efficient implementations * Use Vector for reversing * Remove redundant benchmarking methods * Format scaladoc consistently * Rename snoc and cons to append and prepend for consistency * Add proper Eq, PartialOrder, Order and Coflatmap instances
Adds Catenable from fs2 and fixes #2358
Still needs: