Add NonEmptyList#partitionE #1858

LukaJCB · 2017-08-26T10:21:38Z

As per #1743 (now closed). Also open for alternative names (partitionIor maybe?)

codecov-io · 2017-08-26T10:55:21Z

Codecov Report

Merging #1858 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #1858      +/-   ##
=========================================
+ Coverage   95.17%   95.2%   +0.02%     
=========================================
  Files         248     248              
  Lines        4352    4379      +27     
  Branches      121     125       +4     
=========================================
+ Hits         4142    4169      +27     
  Misses        210     210

Impacted Files	Coverage Δ
core/src/main/scala/cats/Reducible.scala	`95.38% <100%> (+0.46%)`	⬆️
core/src/main/scala/cats/instances/list.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/Foldable.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyList.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/NonEmptyVector.scala	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef28e24...e1d39b1. Read the comment docs.

kailuowang · 2017-08-28T15:57:50Z

Would it be more useful to add it to NonEmptyTraverse with a slightly different signature?

def partitionIor[B, C](f: A => Ior[B, C]): Ior[NonEmptyList[B], NonEmptyList[C]] = {

LukaJCB · 2017-08-28T19:34:49Z

You mean generalizing it to NonEmptyTraverse?

def partitionIor[F[_]: NonEmptyTraverse, A, B, C](fa: F[A])(f: A => Ior[B, C]): Ior[F[B], F[C]]

Not sure, if that works, but would be cool :)

kailuowang · 2017-08-28T20:27:53Z

@LukaJCB i didn't mean to generalize the result NonEmptyList part (not sure if that's doable). I.e.
for NonEmptyVector it would still return Ior[NonEmptyList[B], NonEmptyList[C]], but at least it will have this method.

johnynek · 2017-08-29T06:06:03Z

You don't need traverse do you? Reducible will do, no?

LukaJCB · 2017-08-29T08:39:08Z

@johnynek is correct! I was able to generalize it to Reducible :)

kailuowang · 2017-08-29T13:44:33Z

looks good. is it just me or does anyone else feel a bit weird that we have a partitionE on Reducible but not a partition on Foldable (on which it would probably be returning (List[A], List[B])

like how about this

  def partitionEither[A, B, C](fa: F[A])(f: A => Either[B, C])(implicit F: MonoidK[F], A: Applicative[F]): (F[B], F[C]) =
    foldRight(fa, Eval.later((F.empty[B], F.empty[C]))) { (a, e) =>
      e.map {
        case (fbs, fcs) => f(a).fold(
          b => (F.combineK(A.pure(b), fbs), fcs),
          c => (fbs, F.combineK(A.pure(c), fcs))
        )
      }
    }.value
}

Note that it requires Applicative and MonoidK on F, maybe we can reduce that requirements?

LukaJCB · 2017-08-29T14:19:39Z

I don't think we can reduce the requirements, because either side of the tuple might be empty, so MonoidK is a must, and for Applicative to go, we'd need a way to go from Either[B,C] to (F[B], F[C]), so either we'd keep those two, or we'd just return (List[A], List[B]) as you said.

kailuowang · 2017-08-29T14:25:55Z

I am fine with keeping those requirements. My guess is that for most of the use cases they are going to have those two instances anyway.

peterneyens · 2017-08-29T14:30:00Z

That Foldable#partitionEither would be the same as a map followed by Alternative#separate, right?
Maybe we can call it Alternative#mapSeparate ?

LukaJCB · 2017-08-29T14:37:26Z

That's a good idea! It also just dawned on me that MonoidK + Applicative is Alternative 🙈. I think the implementation could be even easier:

def mapSeparate[A, B, C](fa: F[A])(f: A => Either[B, C])(implicit A: Alternative[F]): (F[B], F[C]) =
  foldMap(fa)(a => f(a) match {
    case Right(c) => (A.empty[B], A.pure(c))
    case Left(b) => (A.pure(b), A.empty[C])
  })

kailuowang · 2017-08-29T14:45:14Z

Right I keep forgetting Alternative

LukaJCB · 2017-08-29T15:03:09Z

It seems the above didn't work, because there's no Monoid instance only MonoidK. It'd need to be foldMapK.

  def mapSeparate[A, B, C](fa: F[A])(f: A => Either[B, C])(implicit A: Alternative[F]): (F[B], F[C]) = {

    implicit val mFbFC: Monoid[(F[B], F[C])] = new Monoid[(F[B], F[C])] {
      def empty: (F[B], F[C]) = (A.empty[B], A.empty[C])
      def combine(x: (F[B], F[C]), y: (F[B], F[C])): (F[B], F[C]) = (x, y) match {
        case ((xfb, xfc), (yfb, yfc)) => (MonoidK[F].combineK(xfb, yfb), MonoidK[F].combineK(xfc, yfc))
      }
    }

    foldMap(fa)(a => f(a) match {
      case Right(c) => (A.empty[B], A.pure(c))
      case Left(b) => (A.pure(b), A.empty[C])
    })
  }

kailuowang · 2017-08-29T15:07:30Z

can MonoidK#algebra help?

LukaJCB · 2017-08-29T15:46:21Z

algebra works like a charm, thanks! Should I add this to the current PR, or make a new one? :)

kailuowang · 2017-08-29T15:58:22Z

current PR works for me.

kailuowang · 2017-08-29T16:55:58Z

core/src/main/scala/cats/Alternative.scala

+      case Left(b) => (pure(b), empty[C])
+    })
+  }
+


I think @peterneyens was suggesting that
you can just write

import cats.instances.either._ def mapSeparate[A, B, C](fa: F[A])(f: A => Either[B, C])(implicit FM: Monad[F]): (F[B], F[C]) = separate(map(fa)(f))

So instead of requiring F to be Foldable, it requires to be a Monad.
Or we can use your implementation, which requires Foldable, which might be more performant? (traverse once instead three times?)

I still think we should name it partitionEither or something, the idea is to provide a familiar api to users. map and then separate probably don't justify a special delegate method also named mapSeparate.

I think I prefer the Foldable way, for the reasons you mentioned. I'm not married to the name, but it makes sense in the same way flatMap makes sense, I guess.

I think I'd call the method on Foldable partitionEither as you suggested and renamed the one on Reducible to partitionIor Maybe?
partitionE isn't a great name IMO, because there's no real precedence for the "E" and I think if we want to stay a beginner friendly library we should have as little of these one-letter-abbreviations as possible.

The current implementation is better than map + separate indeed. Just wanted to point out the similarity.

separate is available for all Bifoldables while we currently have to use Either with mapSeparate:

With separate you could also split Ior and Tuple2 (similar to unzip).

mapSeparate + toEither will probably be faster than map(toEither).separate as you fold once instead of twice.

I agree that partitionE isn't a great name.

my 2 cents

since we are going to use Foldable, it seems to make more sense to be on Foldable than Alternative. Having a pair of partition methods on Foldable and Reducible is what I was looking for.

both methods should be named partitionEitherXX since they both use a function that returns an Either for the actual partition logic. Maybe we can call Reducible#partitionEitherNE and Foldable#partitionEither

maybe we should mention the map + separate option in the scaladoc in Foldable#partitionEither, I think it might worth some promotion.

I'm +1 on renaming to Foldable#partitionEither and mentioning the map + separate option in the scaladoc. Disagree on partitionEitherNE though, for similar reasons as partitionE.
How about nonEmptyPartition? Would go in line with nonEmptyIntercalate etc. 🤔

I am fine with nonEmptyPartition

Renamed to Foldable#partitionEither and Reducible#nonEmptyPartition :)

peterneyens · 2017-08-30T11:55:24Z

tests/src/test/scala/cats/tests/ReducibleTests.scala

+      val h: Int => Either[String, Double] = f andThen Left.apply
+
+      val withG = fi.partitionE(g).fold(_ => NonEmptyList.one(""), identity, (l,r) => r)
+      withG should === (Reducible[F].toNonEmptyList((fi.map(f))))


If this map is the reason we add the Functor constraint to ReducibleCheck we could write Reducible[F].toNonEmptyList(fi).map(f), right?

peterneyens · 2017-08-30T11:57:41Z

tests/src/test/scala/cats/tests/ReducibleTests.scala

+      val g: Int => Either[Double, String] = f andThen Right.apply
+      val h: Int => Either[String, Double] = f andThen Left.apply
+
+      val withG = fi.partitionE(g).fold(_ => NonEmptyList.one(""), identity, (l,r) => r)


The fold could be .right.getOrElse(NonEmptyList.one("")).

peterneyens · 2017-08-30T12:12:40Z

core/src/main/scala/cats/Alternative.scala

+      case Left(b) => (pure(b), empty[C])
+    })
+  }
+


The current implementation is better than map + separate indeed. Just wanted to point out the similarity.

separate is available for all Bifoldables while we currently have to use Either with mapSeparate:

With separate you could also split Ior and Tuple2 (similar to unzip).

mapSeparate + toEither will probably be faster than map(toEither).separate as you fold once instead of twice.

I agree that partitionE isn't a great name.

peterneyens · 2017-08-30T12:15:58Z

core/src/main/scala/cats/Reducible.scala

+    import cats.syntax.either._
+
+    def g(a: A, eval: Eval[Ior[NonEmptyList[B], NonEmptyList[C]]]): Eval[Ior[NonEmptyList[B], NonEmptyList[C]]] = {
+      val ior = eval.value


We should probably do:

eval.map { ior => (f(a), ior) match { // ... } }

LukaJCB · 2017-08-30T14:10:58Z

Thanks for your review @peterneyens! I addressed your comments :)

kailuowang · 2017-08-30T14:42:43Z

tests/src/test/scala/cats/tests/FoldableTests.scala

@@ -25,6 +25,40 @@ abstract class FoldableCheck[F[_]: Foldable](name: String)(implicit ArbFInt: Arb
    }
  }

+  test("Alternative#partitionEither retains size") {


titles of the tests should be Foldable#partitionEither xxxx

kailuowang · 2017-08-30T14:44:01Z

core/src/main/scala/cats/Foldable.scala

@@ -407,6 +407,21 @@ import simulacrum.typeclass
    }.toList

  /**
+    * Separate this Foldable into a Tuple by a separating function `A => Either[B, C]`
+    */
+  def partitionEither[A, B, C](fa: F[A])(f: A => Either[B, C])(implicit F: Foldable[F], A: Alternative[F]): (F[B], F[C]) = {


you are already in Foldable, so no need for the implicit F: Foldable[F] here

peterneyens · 2017-08-30T17:12:54Z

tests/src/test/scala/cats/tests/FoldableTests.scala

+    }
+  }
+
+  test("Foldable#mapSeparate remains sorted") {


You forgot to rename it here.

peterneyens · 2017-08-30T17:16:26Z

core/src/main/scala/cats/Foldable.scala

@@ -407,6 +407,31 @@ import simulacrum.typeclass
    }.toList

  /**
+    * Separate this Foldable into a Tuple by a separating function `A => Either[B, C]`
+    * Equivalent to `Functor#map` and then `Alternative#separate`.


If we did define it in Alternative, we could add a consistency law checking this equality, but hey ... you can't always get what you want 😃

True, I don't really mind either way tbh.
We could add a test that checks if it's consistent with List#partition though 😄

fommil · 2017-09-03T13:25:11Z

@LukaJCB since I wrote this, I'd appreciate a mention in the method 😉 recall that my original implementation carried no license with it.

fommil · 2017-09-03T13:27:14Z

hmm, on second thoughts, your impl is sufficiently different that I don't consider it a derivative works, no attribution required.

LukaJCB · 2017-09-03T14:06:55Z

Hey @fommil sorry for any confusion, didn't mean any harm!

kailuowang · 2017-09-04T18:03:53Z

@peterneyens any other feedback?

johnynek · 2017-09-05T19:39:56Z

core/src/main/scala/cats/Reducible.scala

+    * res1: cats.data.Ior[cats.data.NonEmptyList[Nothing],cats.data.NonEmptyList[Int]] = Right(NonEmptyList(4, 8, 12, 16))
+    * }}}
+    */
+  def nonEmptyPartition[A, B, C](fa: F[A])(f: A => Either[B, C]): Ior[NonEmptyList[B], NonEmptyList[C]] = {


couldn't we do this considerably more efficiently for NonEmptyList and NonEmptyVector? Can we add methods to those datatypes that does so? Eval is kind of a perf killer, sadly, which this method hides.

I just had the same idea :D I already implemented a O(n) solution for partitionEither on List and will override nonemptyPartition as well!

Added overrides for NEL and NEV :)

johnynek · 2017-09-06T18:57:38Z

tests/src/test/scala/cats/tests/ReducibleTests.scala

+    }
+  }
+
+  test("Reducible#nonEmptyPartition remains sorted") {


this is a bit weird. The only F[_] part of it toNonEmptyList. After that, we are testing nonEmptyPartition on NonEmptyList, no?

Yes that's true, I wrote this test when originally nonEmptyPartition was part of NonEmptyList. I could move the test there, if you'd like :) Otherwise, I'm not sure it's possible to sort an F[_] with just Reducible

johnynek · 2017-09-06T19:53:43Z

👍

kailuowang · 2017-09-06T20:41:08Z

Great contribution. Thanks @LukaJCB !

LukaJCB force-pushed the add-partitione-nel branch from 2f25872 to 3be4721 Compare August 29, 2017 10:18

kailuowang reviewed Aug 29, 2017

View reviewed changes

kailuowang added the enhancement label Aug 29, 2017

peterneyens reviewed Aug 30, 2017

View reviewed changes

LukaJCB force-pushed the add-partitione-nel branch from d40c925 to 1f30a1c Compare August 30, 2017 14:10

kailuowang reviewed Aug 30, 2017

View reviewed changes

LukaJCB force-pushed the add-partitione-nel branch from d00a55f to c7f3b69 Compare August 30, 2017 15:14

peterneyens reviewed Aug 30, 2017

View reviewed changes

kailuowang approved these changes Aug 30, 2017

View reviewed changes

fommil mentioned this pull request Sep 3, 2017

partition or partitionE on Foldable? scalaz/scalaz#1448

Closed

johnynek reviewed Sep 5, 2017

View reviewed changes

LukaJCB force-pushed the add-partitione-nel branch 2 times, most recently from dcb2852 to 505475d Compare September 6, 2017 08:11

johnynek reviewed Sep 6, 2017

View reviewed changes

Luka Jacobowitz added 18 commits September 6, 2017 21:42

Add NonEmptyList#partitionE

0f9cd55

Generalize partitionE to Reducible

47048a9

Add doctest

3de168a

Add mapSeparate

a5cbeee

Fix nitpick

e4029ab

Add Scaladoc and tests for mapSeparate

0a7ac70

Simplify

dbd636e

Rename operations

120ee76

Rename test titles as well

9949687

Remove redundant Foldable constraint

f3169aa

Add doc tests and note in docs for map + separate

3c069d2

Forgot to rename

98dfc73

Add test checking if partitionEither is consistent with List#partition

fc825c4

Override Foldable[List].partitionEither for performance

202abbc

add more efficient overloads

49891e1

Add serializable

2510240

Move sorted test to NonEmptyList

ed75575

Replicate sorted test to NEV and NES

e1d39b1

LukaJCB force-pushed the add-partitione-nel branch from 07d5a6a to e1d39b1 Compare September 6, 2017 19:49

kailuowang merged commit 7be0e64 into typelevel:master Sep 6, 2017

LukaJCB deleted the add-partitione-nel branch September 7, 2017 08:07

kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017

Add NonEmptyList#partitionE #1858

Add NonEmptyList#partitionE #1858

Conversation

LukaJCB commented Aug 26, 2017 • edited Loading

codecov-io commented Aug 26, 2017 • edited Loading

Codecov Report

kailuowang commented Aug 28, 2017

LukaJCB commented Aug 28, 2017

kailuowang commented Aug 28, 2017 • edited Loading

johnynek commented Aug 29, 2017

LukaJCB commented Aug 29, 2017

kailuowang commented Aug 29, 2017

LukaJCB commented Aug 29, 2017

kailuowang commented Aug 29, 2017

peterneyens commented Aug 29, 2017

LukaJCB commented Aug 29, 2017 • edited Loading

kailuowang commented Aug 29, 2017

LukaJCB commented Aug 29, 2017 • edited Loading

kailuowang commented Aug 29, 2017

LukaJCB commented Aug 29, 2017

kailuowang commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukaJCB commented Aug 30, 2017

Choose a reason for hiding this comment

kailuowang Aug 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fommil commented Sep 3, 2017

fommil commented Sep 3, 2017

LukaJCB commented Sep 3, 2017

kailuowang commented Sep 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Sep 6, 2017

kailuowang commented Sep 6, 2017

LukaJCB commented Aug 26, 2017 •

edited

Loading

codecov-io commented Aug 26, 2017 •

edited

Loading

kailuowang commented Aug 28, 2017 •

edited

Loading

LukaJCB commented Aug 29, 2017 •

edited

Loading

LukaJCB commented Aug 29, 2017 •

edited

Loading

kailuowang Aug 30, 2017 •

edited

Loading

kailuowang commented Sep 4, 2017 •

edited

Loading