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.

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

Add filterFold and collectFold to Foldable #2452

Merged
merged 9 commits into from
Sep 12, 2018

Conversation

kun-song
Copy link
Contributor

@kun-song kun-song commented Sep 1, 2018

fix #2322

def filterFold[A, M: Monoid](fa: F[A])(f: A ⇒ Option[M]): M = {
val m = Monoid[M]
foldLeft(fa, m.empty)((acc, a) ⇒ f(a) match {
case Some(x) ⇒ m.combine(x, acc)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the order on combine is wrong. I think this won’t match collectFold for a non commutative Monoid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's right, thanks, I will fix it soon :)

test(s"Foldable[$name] partial summation") {
forAll { (fa: F[Int], f: Int ⇒ Boolean) ⇒
val m: Monoid[Int] = Monoid[Int]

Copy link
Contributor

Choose a reason for hiding this comment

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

Let’s test with a non commutative Monoid like String.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, l will do it, thanks.

@kun-song
Copy link
Contributor Author

kun-song commented Sep 2, 2018

The CI complains an binary compatibility issue:

[error]  * method filterFold(java.lang.Object,scala.Function1,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable.filterFold")
[error]  * method collectFold(java.lang.Object,scala.PartialFunction,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable.collectFold")
[error]  * method filterFold(scala.Function1,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable#Ops is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable#Ops.filterFold")
[error]  * method collectFold(scala.PartialFunction,cats.kernel.Monoid)java.lang.Object in trait cats.Foldable#Ops is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("cats.Foldable#Ops.collectFold")

Should I implement these two methods as syntax extensions to avoid this problem?

@codecov-io
Copy link

codecov-io commented Sep 2, 2018

Codecov Report

Merging #2452 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2452     +/-   ##
=========================================
+ Coverage   95.29%   95.39%   +0.1%     
=========================================
  Files         351      357      +6     
  Lines        6371     6519    +148     
  Branches      279      275      -4     
=========================================
+ Hits         6071     6219    +148     
  Misses        300      300
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/foldable.scala 100% <100%> (ø) ⬆️
core/src/main/scala/cats/instances/list.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/option.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/data/Const.scala 100% <0%> (ø) ⬆️
core/src/main/scala/cats/instances/vector.scala 100% <0%> (ø) ⬆️
.../src/main/scala/cats/laws/TraverseFilterLaws.scala 100% <0%> (ø)
...s/src/main/scala/cats/laws/FunctorFilterLaws.scala 100% <0%> (ø)
...cala/cats/laws/discipline/FunctorFilterTests.scala 100% <0%> (ø)
...ala/cats/laws/discipline/TraverseFilterTests.scala 100% <0%> (ø)
core/src/main/scala/cats/FunctorFilter.scala 100% <0%> (ø)
... and 7 more

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 6ac776c...46d92cc. Read the comment docs.

LukaJCB
LukaJCB previously approved these changes Sep 2, 2018
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

I think it looks good for now, I'd like to add more laws like the one I mentioned in the issue, but since we can't add these two methods to the type class I'm more than content to just merge this as is :)

* res0: Int = 6
*}}}
*/
def filterFold[M: Monoid](f: A ⇒ Option[M])(implicit F: Foldable[F]): M = {
Copy link
Contributor

Choose a reason for hiding this comment

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

def filterFold[M: Monoid](f: A  Option[M])(implicit F: Foldable[F]): M =
  collectFold(Function.unlift(f))

Copy link
Contributor

Choose a reason for hiding this comment

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

or, the other way around:

def collectFold[M: Monoid](f: PartialFunction[A, M])(implicit F: Foldable[F]): M =
  filterFold(f.lift)

Copy link
Member

Choose a reason for hiding this comment

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

We could indeed define these in terms of each other. I'm not sure if they'd be more performant than just using foldLeft, but if we do this, then it should be filterFold in terms of collectFold, since A => Option[M] has to allocate an extra Option :)

Up to you, @satansk :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @PeterPerhac @LukaJCB , I think define them in terms of each other is good, but people are more familiar with foldLeft, so I personally prefer to define them by foldLeft :)

But if there are big performance differences, we can change to the better one.

*}}}
*/
def collectFold[M: Monoid](f: PartialFunction[A, M])(implicit F: Foldable[F]): M = {
val m = Monoid[M]
Copy link
Contributor

Choose a reason for hiding this comment

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

totally nitpick - there are two styles of implicit type class instance summoning in this one method (and the filterFold method below). I think it would help readability a little bit if we just use (implicit F: Foldable[F], M: Monoid[M])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a nice advice, it will be more readable, and I'll fix it :)

Thanks!

* res0: Int = 6
*}}}
*/
def filterFold[M](f: A ⇒ Option[M])(implicit F: Foldable[F], M: Monoid[M]): M =
Copy link
Contributor

Choose a reason for hiding this comment

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

The word filter, to me, kind of implies using a boolean value.
We already have a collectFirstSome https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/Foldable.scala#L233
how about naming this one collectSomeFold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A reasonable advice, I'll fix it.

Copy link
Member

Choose a reason for hiding this comment

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

We do have mapFilter which does use Option though.
Maybe this should be mapFilterFold? I don't know

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I am fine with mapFilterFold too. Sorry about the bikeshedding @satansk , your call.

@LukaJCB LukaJCB merged commit 30c29e8 into typelevel:master Sep 12, 2018
@LukaJCB
Copy link
Member

LukaJCB commented Sep 12, 2018

👍

@kailuowang kailuowang added this to the 1.4 milestone Sep 12, 2018
@He-Pin
Copy link

He-Pin commented Sep 12, 2018

@satansk +100 :)

@kun-song
Copy link
Contributor Author

@hepin1989 :)

@kun-song kun-song deleted the add-partial-folding-to-Foldable branch September 13, 2018 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add partial folding methods to Foldable
7 participants