Add `replicateA_`, `parReplicateA_` #4208

rabinarai1 · 2022-05-20T11:11:30Z

This work was inspired by reading the cats-effect code. We saw that replicateA_ had a TODO message to PR the function to cats, so we decided to try and implement that. We found this difficult to implement in a way that didn't use lots of memory or stack. Then we saw that replicateA_ is missing from other strict functional programming languages like pureScript. So we decided to implement this in the Monad class using tailRecM.

Contributed on behalf of the @opencastsoftware open source team 👨‍💻

armanbilge · 2022-05-20T12:07:21Z

Thanks for the PR! Haven't had a chance to look yet, but wanted to link to:

replicateA_, replicateM, replicateM_ #3705

johnynek · 2022-05-20T18:28:35Z

I think a stack safe and fast replicateA_ is just:

def replicateA_[A](n: Int, fa: F[A]): F[Unit] = {
  val fvoid = void(fa)
  def loop(n: Int): F[Unit] =
    if (n <= 0) unit
    else if (n == 1) fvoid
    else {
      val half = loop(n >> 1)
      val both = productR(half, half)
      if ((n & 1) == 1) productR(both, fvoid)
      else both
    }

  loop(n)
}

This isn't tail recursion, but it is constant depth recursion (it goes at most depth 31 since you are using an Int).

This produces a tree, so it should be shallow depth for the F[_] context as well.

I would prefer we add this to Applicative and not bring Monad into the picture.

rabinarai1 · 2022-05-23T14:01:29Z

@johnynek Thank you for that, I have edited the code to reflect your suggestion! :)

armanbilge

Looking good! Can we add a law check replicateA_(n, fa) <-> replicateA(n, fa).void?

Btw @johnynek is there rationale to make fa: => F[A] lazy for when n == 0?

djspiewak · 2022-05-23T16:57:27Z

This would also imply that parReplicateA is possible, which sounds cool.

johnynek · 2022-05-23T17:31:22Z

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

+  test("replicateA_ executes the Applicative action 'fa' 'n' times") {
+    val A = Applicative[Option]
+    val fa = A.pure(0)
+    assert(fa.replicateA_(5) === Option(unit))


Just a minor bit: can we exercise values other than 5?

Actually, can you make a loop and try all values from 0 up to 10 or something?

johnynek · 2022-05-23T17:33:51Z

Looking good! Can we add a law check replicateA_(n, fa) <-> replicateA(n, fa).void?

+1

Btw @johnynek is there rationale to make fa: => F[A] lazy for when n == 0?

I wouldn't because it wouldn't match replicateA and I assume n == 0 is a very rare case.

rabinarai1 · 2022-05-24T14:43:23Z

laws/src/main/scala/cats/laws/ApplicativeLaws.scala

@@ -59,6 +59,9 @@ trait ApplicativeLaws[F[_]] extends ApplyLaws[F] {
  def applicativeUnit[A](a: A): IsEq[F[A]] =
    F.unit.map(_ => a) <-> F.pure(a)

+  def replicateAVoidReplicateA_Consistent[A](fa: F[A]): IsEq[F[Unit]] =
+    F.replicateA_(2, fa) <-> F.replicateA(2, fa).void


When increasing this number, it causes us to produce large structures in memory when running the tests which caused out of memory errors.

Wow, even to 3? Even still, I think we should take n: Int as a parameter to the law, and let the tests decide what number to put there.

I think we tried 5 and explosions resulted - if you think about this it makes sense as if the input data structure that is being replicated is already huge, replicateA(5).void will replicate it fivefold!

How do you want to plumb this number though the laws? Should these laws accept an explicit parameter now? I don't think that would be binary compatible either.

It's binary-compatible to add n as a parameter here, after all we are defining a brand new method! :)

What I'm suggesting is we leave the laws general, and do the hardcoding in the tests.

Ohh I see, I thought you were suggesting to add this parameter to ApplicativeTests.applicative

@rabinarai1 I think if we can make n a parameter of the law then this should be good to go.

I think for the tests it would be good to use a value bigger than n = 2. To keep the structures from getting too large, I wonder if we can use scalacheck tricks like withSize.

@armanbilge we used Gen.resize to resize the generator of F[A]. But even increasing the number to 5 causes out of memory issues.

rabinarai1 · 2022-05-24T14:44:03Z

I've amended the code to address all the comments left :) thank you

armanbilge · 2022-05-24T14:44:24Z

laws/src/main/scala/cats/laws/discipline/ApplicativeTests.scala

@@ -43,6 +43,7 @@ trait ApplicativeTests[F[_]] extends ApplyTests[F] {
    EqFB: Eq[F[B]],
    EqFC: Eq[F[C]],
    EqFABC: Eq[F[(A, B, C)]],
+    EqFUnit: Eq[F[Unit]],


Oh no, I don't think this change is binary-compatible :(

I wonder if we can fix this by deriving:

def EqFUnit(a: A)(implicit Eq[F[A]]): Eq[F[Unit]] = Eq.by(_.as(a))

And then get an A via Arbitrary[A], since we already have Eq[F[A]]

DavidGregory084 · 2022-05-27T11:43:11Z

laws/src/main/scala/cats/laws/discipline/ApplicativeTests.scala

@@ -54,9 +57,15 @@ trait ApplicativeTests[F[_]] extends ApplyTests[F] {
      "applicative map" -> forAll(laws.applicativeMap[A, B] _),
      "applicative unit" -> forAll(laws.applicativeUnit[A] _),
      "ap consistent with product + map" -> forAll(laws.apProductConsistent[A, B] _),
+      "replicateA_ consistent with replicateA.void" -> forAll { (a: A) =>
+        // Should be an implicit parameter but that is not a binary-compatible change


Is there some place that these bincompat hacks are being catalogued? Should we make this a // TODO: cats 3.x comment?

armanbilge · 2022-05-31T04:37:49Z

laws/src/main/scala/cats/laws/ApplicativeLaws.scala

@@ -59,6 +59,9 @@ trait ApplicativeLaws[F[_]] extends ApplyLaws[F] {
  def applicativeUnit[A](a: A): IsEq[F[A]] =
    F.unit.map(_ => a) <-> F.pure(a)

+  def replicateAVoidReplicateA_Consistent[A](fa: F[A]): IsEq[F[Unit]] =
+    F.replicateA_(2, fa) <-> F.replicateA(2, fa).void


@rabinarai1 I think if we can make n a parameter of the law then this should be good to go.

I think for the tests it would be good to use a value bigger than n = 2. To keep the structures from getting too large, I wonder if we can use scalacheck tricks like withSize.

armanbilge · 2022-06-07T15:48:36Z

Needs a scalafmtAll :)

armanbilge · 2022-06-07T15:50:39Z

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

+      assert(fa.replicateA_(num) === Option(unit))
+      assert(increment.replicateA_(num).runS(0).value === num)
+      assert(increment.replicateA_(num).run(0).value === ((num, ())))
+      assert(increment.replicateA_(num).run(0).value === increment.replicateA(num).void.run(0).value)


Suggested change

assert(fa.replicateA_(num) === Option(unit))

assert(increment.replicateA_(num).runS(0).value === num)

assert(increment.replicateA_(num).run(0).value === ((num, ())))

assert(increment.replicateA_(num).run(0).value === increment.replicateA(num).void.run(0).value)

assertEquals(fa.replicateA_(num), Option(unit))

assertEquals(increment.replicateA_(num).runS(0).value, num)

assertEquals(increment.replicateA_(num).run(0).value, ((num, ())))

assertEquals(increment.replicateA_(num).run(0).value, increment.replicateA(num).void.run(0).value)

armanbilge · 2022-06-08T12:53:51Z

laws/src/main/scala/cats/laws/discipline/ApplicativeTests.scala

@@ -54,9 +57,15 @@ trait ApplicativeTests[F[_]] extends ApplyTests[F] {
      "applicative map" -> forAll(laws.applicativeMap[A, B] _),
      "applicative unit" -> forAll(laws.applicativeUnit[A] _),
      "ap consistent with product + map" -> forAll(laws.apProductConsistent[A, B] _),
+      "replicateA_ consistent with replicateA.void" -> forAll { (a: A) =>
+        // Should be an implicit parameter but that is not a binary-compatible change
+        implicit val eqFUnit = makeEqFUnit[A](a)


Looks like the Scala 3 compiler is tripping on this line.

Ah yeah, looks like we need

Suggested change

implicit val eqFUnit = makeEqFUnit[A](a)

implicit val eqFUnit: Eq[F[Unit]] = makeEqFUnit[A](a)

armanbilge · 2022-06-08T13:29:56Z

Huh, this is 2.12 btw.

[error] /home/runner/work/cats/cats/tests/shared/src/test/scala/cats/tests/SyntaxSuite.scala:419:26: value replicateA_ is not a member of type parameter F[A]
[error]     val replicateA_ = fa.replicateA_(1)

DavidGregory084 · 2022-06-08T13:47:35Z

Huh, this is 2.12 btw.

[error] /home/runner/work/cats/cats/tests/shared/src/test/scala/cats/tests/SyntaxSuite.scala:419:26: value replicateA_ is not a member of type parameter F[A]
[error]     val replicateA_ = fa.replicateA_(1)

Hmm that is very strange indeed as the same code compiled at c4e960d

DavidGregory084 · 2022-06-09T16:05:56Z

Had a proper look at this and I don't get this compilation error locally - @rabinarai1 how about if we just close and reopen the PR to kick off CI again? If that doesn't work, perhaps it's a conflict that's only introduced on merging into main.

DavidGregory084 · 2022-06-09T16:17:27Z

Aha, I was able to replicate it locally after merging main though 👍

armanbilge · 2022-06-09T16:25:07Z

@DavidGregory084 thanks for tracking that down! While you are here, feel free to add parReplicateA_ roundabouts here if you wish :)

cats/core/src/main/scala/cats/syntax/parallel.scala

Lines 294 to 296 in 7ad08aa

    
             def parReplicateA(n: Int)(implicit P: Parallel[M]): M[List[A]] = 
        
               Parallel.parReplicateA(n, ma) 
        
           }

armanbilge · 2022-06-09T16:42:47Z

Fantastic, thanks!!! 🚀

rabinarai1 added 2 commits May 20, 2022 12:04

ReplicateM_

4742ca2

Reformat code

fc1576b

Move back to Applicative as replicateA_

c4e960d

rabinarai1 changed the title ~~ReplicateM_~~ ReplicateA_ May 23, 2022

armanbilge reviewed May 23, 2022

View reviewed changes

johnynek requested changes May 23, 2022

View reviewed changes

Addressed PR comments

944810c

rabinarai1 commented May 24, 2022

View reviewed changes

rabinarai1 requested a review from johnynek May 24, 2022 14:44

armanbilge reviewed May 24, 2022

View reviewed changes

rabinarai1 requested a review from armanbilge May 24, 2022 14:44

Amended to address comments

04ca24a

DavidGregory084 reviewed May 27, 2022

View reviewed changes

armanbilge requested changes May 31, 2022

View reviewed changes

Amended following suggestion

e37d37b

armanbilge reviewed Jun 7, 2022

View reviewed changes

addressed comments

42c2543

armanbilge reviewed Jun 8, 2022

View reviewed changes

Update ApplicativeTests.scala

17eed38

armanbilge closed this Jun 9, 2022

armanbilge reopened this Jun 9, 2022

Merge branch 'main' of github.com:typelevel/cats into ReplicateM_

a7c5ee9

Move replicateA_ extension method into new ApplicativeByValueOps

0a776c7

Add Parallel.parReplicateA_

3b7f9b7

armanbilge changed the title ~~ReplicateA_~~ Add replicateA_, partReplicateA_ Jun 9, 2022

armanbilge changed the title ~~Add replicateA_, partReplicateA_~~ Add replicateA_, parReplicateA_ Jun 9, 2022

armanbilge approved these changes Jun 9, 2022

View reviewed changes

johnynek approved these changes Jun 9, 2022

View reviewed changes

johnynek merged commit b351d67 into typelevel:main Jun 9, 2022

DavidGregory084 deleted the ReplicateM_ branch June 9, 2022 17:18

armanbilge mentioned this pull request Jun 9, 2022

replicateA_, replicateM, replicateM_ #3705

Closed

8 tasks

armanbilge added this to the 2.8.0 milestone Jun 9, 2022

johnynek mentioned this pull request Jun 12, 2022

Optimize replicateA to have same recursive structure as replicateA_ #4233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `replicateA_`, `parReplicateA_` #4208

Add `replicateA_`, `parReplicateA_` #4208

rabinarai1 commented May 20, 2022

armanbilge commented May 20, 2022

johnynek commented May 20, 2022

rabinarai1 commented May 23, 2022

armanbilge left a comment

djspiewak commented May 23, 2022

johnynek May 23, 2022

johnynek commented May 23, 2022

rabinarai1 May 24, 2022

armanbilge May 24, 2022

DavidGregory084 May 27, 2022 •

edited

DavidGregory084 May 27, 2022

armanbilge May 27, 2022

DavidGregory084 May 27, 2022

armanbilge May 31, 2022

rabinarai1 Jun 7, 2022

rabinarai1 commented May 24, 2022

armanbilge May 24, 2022

armanbilge May 24, 2022

DavidGregory084 May 27, 2022

armanbilge May 31, 2022

armanbilge commented Jun 7, 2022

armanbilge Jun 7, 2022

armanbilge Jun 8, 2022

DavidGregory084 Jun 8, 2022

armanbilge commented Jun 8, 2022

DavidGregory084 commented Jun 8, 2022

DavidGregory084 commented Jun 9, 2022

DavidGregory084 commented Jun 9, 2022

armanbilge commented Jun 9, 2022 •

edited

armanbilge commented Jun 9, 2022

	implicit val eqFUnit = makeEqFUnit[A](a)
	implicit val eqFUnit: Eq[F[Unit]] = makeEqFUnit[A](a)

Add replicateA_, parReplicateA_ #4208

Add replicateA_, parReplicateA_ #4208

Conversation

rabinarai1 commented May 20, 2022

armanbilge commented May 20, 2022

johnynek commented May 20, 2022

rabinarai1 commented May 23, 2022

armanbilge left a comment

Choose a reason for hiding this comment

djspiewak commented May 23, 2022

Choose a reason for hiding this comment

johnynek commented May 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DavidGregory084 May 27, 2022 • edited

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

rabinarai1 commented May 24, 2022

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

armanbilge commented Jun 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge commented Jun 8, 2022

DavidGregory084 commented Jun 8, 2022

DavidGregory084 commented Jun 9, 2022

DavidGregory084 commented Jun 9, 2022

armanbilge commented Jun 9, 2022 • edited

armanbilge commented Jun 9, 2022

Add `replicateA_`, `parReplicateA_` #4208

Add `replicateA_`, `parReplicateA_` #4208

DavidGregory084 May 27, 2022 •

edited

armanbilge commented Jun 9, 2022 •

edited