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 TraverseFilter.mapAccumulateFilter #4561

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Masynchin
Copy link
Contributor

If Traverse has mapAccumulate, why don't TraverseFilter has mapAccumulateFilter?

Now it has. I implement method itself (almost fully copied from mapAccumulate, changing traverse to traverseFilter), syntax ops, and tests (almost fully copied from mapAccumulate, adjusting fn signature)

Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time!

I think if we add this we should make it fast for as many instances in cats that we can.

@@ -122,6 +122,9 @@ trait TraverseFilter[F[_]] extends FunctorFilter[F] {
override def mapFilter[A, B](fa: F[A])(f: A => Option[B]): F[B] =
traverseFilter[Id, A, B](fa)(f)

def mapAccumulateFilter[S, A, B](init: S, fa: F[A])(f: (S, A) => (S, Option[B])): (S, F[B]) =
traverseFilter(fa)(a => State(s => f(s, a))).run(init).value
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we override this for some of the built in collections? State is rather slow so we should avoid it for List, Vector, Chain, NonEmptyList, NonEmptyVector, ...

Copy link
Contributor Author

@Masynchin Masynchin Feb 19, 2024

Choose a reason for hiding this comment

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

I can copy StaticMethods.mapAccumulateFromStrictFunctor for mapAccumulateFilter, this will cover for some collections

@Masynchin
Copy link
Contributor Author

There is no override of mapAccumulate for Seq, so should it be added with mapAccumulateFilter through StaticMethods.mapAccumulateFilterFromStrictFunctorFilter?

@Masynchin
Copy link
Contributor Author

Masynchin commented Feb 19, 2024

There is no override of mapAccumulate for Seq, so should it be added with mapAccumulateFilter through StaticMethods.mapAccumulateFilterFromStrictFunctorFilter?

Same for LazyList and Stream

johnynek
johnynek previously approved these changes Feb 22, 2024
Copy link
Contributor

@johnynek johnynek left a comment

Choose a reason for hiding this comment

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

Thanks for sending this PR

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate? I think I prefer the latter.

@Masynchin
Copy link
Contributor Author

Masynchin commented Mar 2, 2024

Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate?

I just added "Filter"-suffix after original method name from Traverse, same as in traversetraverseFilter and sequencesequenceFilter

I have no opinion here, I don't mind both

@@ -122,6 +122,9 @@ trait TraverseFilter[F[_]] extends FunctorFilter[F] {
override def mapFilter[A, B](fa: F[A])(f: A => Option[B]): F[B] =
traverseFilter[Id, A, B](fa)(f)

def mapAccumulateFilter[S, A, B](init: S, fa: F[A])(f: (S, A) => (S, Option[B])): (S, F[B]) =
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that a scaladoc comment (perhaps, with a short usage example) wouldn't hurt and could come handy here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost done, I should fix the code violations. Is the language correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@satorg
Copy link
Contributor

satorg commented Apr 5, 2024

There is no override of mapAccumulate for Seq, so should it be added with mapAccumulateFilter through StaticMethods.mapAccumulateFilterFromStrictFunctorFilter?

I think it can be addressed in a separate follow-up PR, just to avoid blowing up the scope of this one.

@satorg
Copy link
Contributor

satorg commented Apr 5, 2024

  • Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate? I think I prefer the latter.
  • I just added "Filter"-suffix after original method name from Traverse, same as in traverse → traverseFilter and sequence → sequenceFilter

We just get to decide, whether the new method is likely mapFilter + Accumulate or mapAccumulate + Filter 😄

As a bit more crazy idea (maybe not that crazy though): we can add two functions instead:

def mapAccumulateFilter[S, A, B](init: S, fa: F[A])(f: (S, A) => (S, Option[B])): (S, F[B])
def mapFilterAccumulate[S, A, B](init: S, fa: F[A])(f: (S, A) => Option[(S, B)]): (S, F[B])

The former always does accumulate regardless of filter, hence the name.
The latter always does filter first, then accumulate if not filtered out.

Of course, we can implement mapFilterAccumulate in terms of mapAccumulateFilter, e.g.:

def mapFilterAccumulate[S, A, B](init: S, fa: F[A])(f: (S, A) => Option[(S, B)]): (S, F[B]) =
  mapAccumulateFilter(init, fa) { (s1, a) =>
    f(s1, a) match {
      case (s2, Some(b)) => Some((s2, b))
      case (_, None) => None // drop `S` from this iteration
    }
  }

However both functions can come handy under certain circumstances.
(sorry if my snippet is incorrect – it is beyond 1 am already and I am gravily sleepy)

Moreover, even more important – it could help up to avoid choosing between two really good names and just move on with this PR :)

@Masynchin
Copy link
Contributor Author

Of course, we can implement mapFilterAccumulate in terms of mapAccumulateFilter, e.g.:

def mapFilterAccumulate[S, A, B](init: S, fa: F[A])(f: (S, A) => Option[(S, B)]): (S, F[B]) =
  mapAccumulateFilter(init, fa) { (s1, a) =>
    f(s1, a) match {
      case (s2, Some(b)) => Some((s2, b))
      case (_, None) => None // drop `S` from this iteration
    }
  }

This is implementation of mapAccumulateFilter in terms of mapFilterAccumulate. f pattern matching should be changed inside out:

f(s1, a) match
  case Some((s2, b)) => (s2, Some(b))
  case None          => (s1, None)

@satorg
Copy link
Contributor

satorg commented Apr 5, 2024

This is implementation of mapAccumulateFilter in terms of mapFilterAccumulate. f pattern matching should be changed inside out:

Yes, thank you 😊

Anyway, mapFilterAccumulate looks like a special case for mapAccumulateFilter, which is more general. But it can come more handy when accumulation is not needed on entries where the condition does not hold.

@armanbilge – wdyt?

@@ -38,6 +38,19 @@ abstract class TraverseFilterSuite[F[_]: TraverseFilter](name: String)(implicit

implicit def T: Traverse[F] = implicitly[TraverseFilter[F]].traverse

test(s"TraverseFilter[$name].mapAccumulateFilter") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it does not check the default implementation (based on State), does it?
Except maybe one for Stream, but I wouldn't count on it.

For testing default implementations we usually use ListWrapper from testkit:
ListWrapper.scala

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had straightforwardly copy-pasted and updated tests from mapAccumulate here:

test(s"Traverse[$name].mapAccumulate") {
forAll { (init: Int, fa: F[Int], fn: ((Int, Int)) => (Int, Int)) =>
val lhs = fa.mapAccumulate(init)((s, a) => fn((s, a)))
val rhs = fa.foldLeft((init, List.empty[Int])) { case ((s1, acc), a) =>
val (s2, b) = fn((s1, a))
(s2, b :: acc)
}
assert(lhs.map(_.toList) === rhs.map(_.reverse))
}
}

If that tests doesn't test default implementation too, I can update mapAccumulateFilter tests. Can you provide an example of how to pass ListWrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For testing default implementations we usually use ListWrapper from testkit: ListWrapper.scala

I am failing to understand how to use ListWrapper to test mapAccumulateFilter. I was looking for examples in other Suites, but it is either suites for data and not typeclasses (OptionT, Try, etc.), or it refers to <Typeclass>Tests[ListWrapper].<methodToTest>, like in the ApplicativeSuite:

implicit val listwrapperApplicative: Applicative[ListWrapper] = ListWrapper.applicative
implicit val listwrapperCoflatMap: CoflatMap[ListWrapper] = Applicative.coflatMap[ListWrapper]
checkAll("Applicative[ListWrapper].coflatMap", CoflatMapTests[ListWrapper].coflatMap[String, String, String])

which I can not apply here, because we don't have TraverseFilter[?].mapAccumulateFilter. Am I missing something?

Copy link
Contributor

@satorg satorg Apr 10, 2024

Choose a reason for hiding this comment

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

My apologies for the delay – I was snowed under a bit. Actually, there's TraverseFilter for ListWrapper:

val traverseFilter: TraverseFilter[ListWrapper] = {
val F = TraverseFilter[List]
new TraverseFilter[ListWrapper] {
def traverse = ListWrapper.traverse
def traverseFilter[G[_], A, B](
fa: ListWrapper[A]
)(f: A => G[Option[B]])(implicit G: Applicative[G]): G[ListWrapper[B]] =
G.map(F.traverseFilter(fa.list)(f))(ListWrapper.apply)
}
}

To test the default implementation you can either call it directly:

ListWrapper.traverseFilter.mapAccumulateFilter(...)

or make it an implicit in the scope:

implicit val listWrapperTraverseFilter: TraverseFilter[ListWrapper] = ListWrapper.traverseFilter

And then you can work with TraverseFilter for ListWrapper as usual.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants