TraverseFilter (AKA Witherable) type class by ceedubs · Pull Request #1148 · typelevel/cats

ceedubs · 2016-06-19T16:02:59Z

This is not meant to be merged in its current form.

This is a proof of concept for adding a TraverseFilter type class, which is a port of Haskell's Data.Witherable. I'm not set on any of the naming, so feel free to offer other recommendations.

Would people be interested in something like this? Some things that I like about it are:

It provides filterM (actually a more general version of it) filterM broken? #1054.
It provides collect and mapFilter (aka mapMaybe) methods collect and mapMaybe for MonadFilter? #842. Since collect is often used on standard library collections, I think this is a nice feature to support.
traverseFilter supports operations that essentially do a filter and a traverse at the same time, which is something that you can't get from either Traverse or MonadFilter.

Some things that I'm not so sure about:

There's definitely some overlap with MonadFilter here. For example, filter and collect can be defined for both. I'm not sure if there should be a closer relationship between the two. Also syntax enrichment for methods like filter could potentially be ambiguous between the two types (though I don't know how much of a problem this will be in practice as most concrete types that support filter will have it defined on them).
(edit: I think this is resolved) I'm not so sure about the composition law. The Haskell wiki defines it as Compose . fmap (wither f) . wither g ≡ wither (Compose . fmap (wither f) . g), but I can't get the types to line up on that. I don't know Haskell very well and struggle with point free style, so I'm probably losing something in translation. I have a different version of the right hand side of the composition law that compiles, but it seems much less direct than the haskell version, and I don't know know if it's meaningfully distinct from the left hand side. I'm hoping someone who knows Haskell well (such as @aaronlevin or @sellout) and/or someone who knows laws well (such as @fthomas) can weigh in on this.

If people want to go forward with this I need to provide more instances, tests, and documentation.

ceedubs · 2016-06-19T16:05:10Z

+   * res0: List[String] = List(one, three)
+   * }}}
+   */
+  def mapOptionA[G[_]: Applicative, A, B](fa: F[A])(f: A => G[Option[B]]): G[F[B]]


I guess another potential name for this would be traverseOption.

codecov-io · 2016-06-19T16:40:42Z

Current coverage is 89.32%

Merging #1148 into master will increase coverage by 0.52%

@@             master      #1148   diff @@
==========================================
  Files           232        238     +6   
  Lines          3073       3195   +122   
  Methods        3021       3139   +118   
  Messages          0          0          
  Branches         49         53     +4   
==========================================
+ Hits           2729       2854   +125   
+ Misses          344        341     -3   
  Partials          0          0

Powered by Codecov. Last updated by 89bc41f...6f7840c

sellout · 2016-06-19T19:50:45Z

+    val rhs: Nested[M, N, F[C]] = fa.mapOptionA[Nested[M, N, ?], C](a =>
+      Nested(f(a).map(_.traverseM(g))))
+    lhs <-> rhs
+  }


So, I haven’t tried it, but the only difference I see between your law and the Haskell one is that the Haskell uses mapOptionA where you use traverseM. And I think the types line up if you make that change.

More superficially, the Haskell version also swaps the names f and g.

@sellout It's an Option[B] that I'm calling traverseM on. I think there's an extra layer of Option if I try to use mapOptionA. It seems off since I'm not actually calling mapOptionA twice.

I think it’s fine that it’s an Option[B] – the type signatures of mapOptionA and traverseM are both F[A] =...> G[F[B]]

def mapOptionA[G[_]: Applicative, A, B](fa: F[A])(f: A => G[Option[B]]): G[F[B]] def traverseM[G[_]: Applicative, A, B](fa: F[A])(f: A => G[F[B]]): G[F[B]]

so in this specific case, they both go Option[B] => (B => N[Option[C]]) => N[Option[C]]. The only difference is how the Option in N[Option[C]] is handled – IIUC, mapOptionA throws away the None cases, while traverseM flatMaps them, which doesn’t throw them away.

Ah so do I need to write an instance for Option and use it in the law? I
assumed I would only need the instance for the abstract F.
On Jun 19, 2016 10:17 PM, "Greg Pfeil" notifications@github.com wrote:

In laws/src/main/scala/cats/laws/CollectLaws.scala
#1148 (comment):

fa: F[A],

f: A => M[Option[B]],

g: B => N[Option[C]]

)(implicit

M: Applicative[M],

N: Applicative[N]

): IsEq[Nested[M, N, F[C]]] = {

val lhs: Nested[M, N, F[C]] = Nested(fa.mapOptionA(f).map(_.mapOptionA(g)))

// TODO is this right and/or meaningful? I can't seem to get the types to

// line up for the a straight port of wither (Compose. fmap (wither f). g)

// which is how the law is stated for Haskell's Witherable

val rhs: Nested[M, N, F[C]] = fa.mapOptionA[Nested[M, N, ?], C](a =>

Nested(f(a).map(_.traverseM(g))))

lhs <-> rhs

}

I think it’s fine that it’s an Option[B] – the type signatures of mapOptionA
and traverseM are both F[A] =...> G[F[B]]

def mapOptionA[G[]: Applicative, A, B](fa: F[A])(f: A =>
G[Option[B]]): G[F[B]]def traverseM[G[]: Applicative, A, B](fa:
F[A])(f: A => G[F[B]]): G[F[B]]

so in this specific case, they both go Option[B] => (B => N[Option[C]]) =>
N[Option[C]]. The only difference is how the Option in N[Option[C]] is
handled – IIUC, mapOptionA throws away the None cases, while traverseM
flatMaps them, which doesn’t throw them away.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/typelevel/cats/pull/1148/files/139659472e3b698dd0e392099f01155f33cbe1b5#r67630044,
or mute the thread
https://github.com/notifications/unsubscribe/AA7sCUSHu09nEV8TRexRAl7JhuoU5XJiks5qNfhMgaJpZM4I5Mbj
.

I actually hadn’t thought about it, but yeah, it looks like that’s the case. It looks like Witherable has an instance for Maybe as well as some others.

Thanks for the help @sellout. I've updated this PR to include what I think is the right representation of the law.

ceedubs · 2016-06-27T12:15:32Z

I renamed the type class from Collect to TraverseFilter, which seems like a slightly better name to me (and relates to MonadFilter). Still open to other name ideas.

Resolves typelevel#1054. This simply removes `MonadFilter.filterM` to prevent confusion due to this `filterM` being different than that from Haskell or Scalaz. This version of `filterM` doesn't seem particularly useful to me. The only type that comes to mind for which this might be useful is `Option`, and it's pretty easy to come up with a more straightforward implementation for the `Option` case. A more general solution for `filterM` is provided by typelevel#1148, but so far there doesn't seem to be any interest in that proposal, so I don't want it to hold up resolving

kailuowang · 2016-07-15T15:28:52Z

Looks great to me. Thanks very much! @ceedubs 👍

johnynek · 2016-07-15T15:34:49Z

+ * that can essentially have a [[traverse]] and a [[filter]] applied as a single
+ * combined operation ([[traverseFilter]]).
+ *
+ * TODO ceedumbs more info


Can we add the laws:

A definition of wither must satisfy the following laws: identity wither (pure . Just) ≡ pure composition Compose . fmap (wither f) . wither g ≡ wither (Compose . fmap (wither f) . g)

By which I mean, mention in the comments the laws, I see they are encoded below in the tests.

johnynek · 2016-07-15T15:36:47Z

This looks really nice.

👍

ceedubs · 2016-07-16T14:58:22Z

      val G = Traverse[G]
    }

+  def composeFilter[G[_]: TraverseFilter]: TraverseFilter[λ[α => F[G[α]]]] =


Are people happy with this? Since we only need a Traverse for F but a TraverseFilter for G, and there is a separate compose when G only has Traverse, it doesn't quite fit the common pattern.

ceedubs · 2016-07-16T15:04:19Z

@kailuowang @johnynek thanks for the reviews! Since you've shown support for this change, I've fleshed it out a bit. Would you mind taking a second look?

I left a couple minor comments seeking feedback, but I think they are for things that could probably be addressed in a later PR.

The one item that I'm still a little uneasy about is this (from the original PR description):

There's definitely some overlap with MonadFilter here. For example, filter and collect can be defined for both. I'm not sure if there should be a closer relationship between the two. Also syntax enrichment for methods like filter could potentially be ambiguous between the two types (though I don't know how much of a problem this will be in practice as most concrete types that support filter will have it defined on them).

ceedubs · 2016-07-23T15:50:11Z

I'm closing this in favor of #1225, where I've cleaned up the git history a bit and added FunctorFilter to resolve the concerns I had raised about overlap between TraverseFilter and MonadFilter.

ceedubs added 3 commits June 19, 2016 08:19

WIP add Collect AKA Witherable

c3efb2a

Add Collect tests

13db35a

Add filter method to Collect

1396594

ceedubs added the in progress label Jun 19, 2016

ceedubs reviewed Jun 19, 2016
View reviewed changes

sellout reviewed Jun 19, 2016
View reviewed changes

ceedubs added 3 commits June 20, 2016 07:52

Fix Collect composition law

fba0bf8

Merge branch 'master' into wip-collect

2f8079a

Rename Collect to TraverseFilter

0c01e2b

ceedubs changed the title ~~Proof of Concept: Collect (AKA Witherable) type class~~ Proof of Concept: TraverseFilter (AKA Witherable) type class Jun 27, 2016

ceedubs mentioned this pull request Jul 15, 2016

Remove MonadFilter.filterM #1202

Merged

johnynek reviewed Jul 15, 2016
View reviewed changes

Add some TraverseFilter instances

513177a

ceedubs reviewed Jul 16, 2016
View reviewed changes

Add missing comma to TraverseFilter doc

6f7840c

ceedubs changed the title ~~Proof of Concept: TraverseFilter (AKA Witherable) type class~~ TraverseFilter (AKA Witherable) type class Jul 19, 2016

ceedubs mentioned this pull request Jul 20, 2016

Use traits to avoid code duplication in Nested instances #1221

Merged

ceedubs closed this Jul 23, 2016

ceedubs removed the in progress label Jul 23, 2016

ceedubs mentioned this pull request Jul 23, 2016

Add FunctorFilter and TraverseFilter #1225

Merged

Uh oh!

Uh oh!

Conversation

ceedubs commented Jun 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 89.32%

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ceedubs commented Jun 27, 2016

Uh oh!

kailuowang commented Jul 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnynek commented Jul 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ceedubs commented Jul 16, 2016

Uh oh!

ceedubs commented Jul 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ceedubs commented Jun 19, 2016 •

edited

Loading

codecov-io commented Jun 19, 2016 •

edited

Loading