Add TraverseFilter.mapAccumulateFilter

[Masynchin]

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)

[johnynek]

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.

[johnynek]

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, ...

[Masynchin]

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

[Masynchin]

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

[Masynchin]

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

Same for LazyList and Stream

[johnynek]

Thanks for sending this PR

[armanbilge]

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

[Masynchin]

Bikeshed question: why mapAccumulateFilter vs mapFilterAccumulate?

I just added "Filter"-suffix after original method name from Traverse, same as in traverse → traverseFilter and sequence → sequenceFilter

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

[satorg]

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

[Masynchin]

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

[satorg]

Looks good to me, thanks!

[satorg]

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]

• 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]

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]

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?

[satorg]

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

[Masynchin]

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

cats/tests/shared/src/test/scala/cats/tests/TraverseSuite.scala

Lines 45 to 56 in 70dbf8f

	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?

[Masynchin]

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:

cats/tests/shared/src/test/scala/cats/tests/ApplicativeSuite.scala

Lines 102 to 104 in 70dbf8f

	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?

[satorg]

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

cats/testkit/src/main/scala/cats/tests/ListWrapper.scala

Lines 80 to 90 in 47fbad7

	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.