SemigroupK/MonoidK methods #4084

TimWSpence · 2021-12-15T12:14:55Z

Ports missing methods from Semigroup/Monoid to SemigroupK/MonoidK

armanbilge

Thanks for the PR! Just a quick question, do we need to add anything to laws to cover these new methods? E.g. to check the consistency of these implementations.

TimWSpence · 2021-12-15T15:15:44Z

Thanks for the PR! Just a quick question, do we need to add anything to laws to cover these new methods? E.g. to check the consistency of these implementations.

Good question! I was waiting to see if the tests passed before commenting 😅 I had two observations to make:

SemigroupK and MonoidK don't have XXXFunctions traits to add forwarders to the companion objects. Should I add those as well?
I was going to comment that these functions for Monoid and Semigroup don't seem to have any tests but now I see that they do in fact have laws so yes, I'll add corresponding laws for these 👍

johnynek

+1 to update the laws.

I think basically we should have laws that the implementations match the naive implementations.

BTW: I think you can override combineAllK for sequence types and get a significant win (list becomes O(N) vs O(N^2)) also Option can short circuit at the first non-empty item.

johnynek · 2021-12-15T17:56:06Z

core/src/main/scala/cats/MonoidK.scala

+   * }}}
+   */
+  override def combineNK[A](a: F[A], n: Int): F[A] =
+    if (n < 0) throw new IllegalArgumentException("Repeated combining for monoids must have n >= 0")


I think we should say MonoidK not monoid here.

Sorry! Good spot!

This hasn't been fixed.

johnynek · 2021-12-15T18:05:18Z

note: https://github.com/typelevel/cats/pull/4084/files#diff-d189c08189989813f9b402a6993d2c49a1b7302601bc5de3dfe287eca63be6beR69

you can override the algebra there to override combineAll and similarly for SemigroupK.

Another idea, with this in hand, I think we can add:

def fromIterableOnce[A](as: IterableOnce[A]): F[A] =
  combineAllK(as.map(pure(_)))

def fromFoldable[G[_]: Foldable, A](as: G[A]): F[A] =
  fromIterableOnce(Foldable[F].toIterable(as))

to Alternative.

TimWSpence · 2021-12-18T16:06:23Z

@johnynek just a heads up that I might not get to this till early in the new year but that it's at the top of my Todo list. Just in case you thought I was ignoring your feedback 😅

johnynek · 2022-01-12T19:00:53Z

core/src/main/scala/cats/MonoidK.scala

+   * }}}
+   */
+  override def combineNK[A](a: F[A], n: Int): F[A] =
+    if (n < 0) throw new IllegalArgumentException("Repeated combining for monoids must have n >= 0")


This hasn't been fixed.

johnynek · 2022-01-12T19:04:36Z

core/src/main/scala/cats/MonoidK.scala

+  def combineAllK[A](as: IterableOnce[F[A]]): F[A] =
+    as.iterator.foldLeft(empty[A])(combineK[A])
+
+  override def combineAllOptionK[A](as: IterableOnce[F[A]]): Option[F[A]] =


I think it is better to do:

def combineAllK(as: IterableOnce[F[A]]): F[A] = combineAllOptionK(as) match { case Some(fa) => fa case None => empty[A] }

and not override combineAllOptionK.

The reason is, you want to only have to override one thing when you implement MonoidK and not have to override both and since combineAllOptionK is on both MonoidK and SemigroupK I think it is the easier one to remember to override.

I have sadly seen many cases of people picking the wrong one to override and the code paths not hitting the optimized method.

Alternatively you could do it this way, but I make this override final (that's allowed right)? so no one accidentally makes the mistake.

Thanks 👍

johnynek · 2022-01-12T19:06:30Z

core/src/main/scala/cats/instances/list.scala

+      }
+
+      override def combineAllK[A](as: IterableOnce[List[A]]): List[A] =
+        as.iterator.flatMap(_.iterator).toList


this duplication is unfortunate IMO. I think in MonoidK we should be able to only override one.

In library code I think it is worth the verbosity for the possibility of slightly better performance:

val bldr = List.newBuilder[A] val it = as.iterator while(it.hasNext) { bldr ++= it.next() } bldr.result()

johnynek · 2022-01-12T19:12:07Z

core/src/main/scala/cats/instances/option.scala

@@ -25,6 +26,12 @@ trait OptionInstances extends cats.kernel.instances.OptionInstances {

      def combineK[A](x: Option[A], y: Option[A]): Option[A] = if (x.isDefined) x else y

+      override def combineAllOptionK[A](as: IterableOnce[Option[A]]): Option[Option[A]] =
+        as.iterator.find(_.isDefined)


I think we should avoid a closure in library code if we can:

val it = as.iterator if (it.hasNext) { while (true) { val o = it.next() if (o.isDefined) return Some(o) if (!it.hasNext) return Some(None) } sys.error("unreachable") } else { return None }

Just for my own edification, why do you prefer to avoid a closure? Just avoiding unnecessary allocations or something deeper?

johnynek · 2022-01-12T19:17:07Z

core/src/main/scala/cats/instances/queue.scala

@@ -17,6 +18,14 @@ trait QueueInstances extends cats.kernel.instances.QueueInstances {

      def combineK[A](x: Queue[A], y: Queue[A]): Queue[A] = x ++ y

+      override def combineAllOptionK[A](as: IterableOnce[Queue[A]]): Option[Queue[A]] = {
+        val iter = as.iterator
+        if (iter.isEmpty) None else Some(iter.flatMap(_.iterator).to(Queue))


I think we could write a function to share like:

def appendAll[F <: Iterable[A], A](as: IterableOnce[F], bldr: Builder[F, A]): bldr.type = { val it = as.iterator while(it.hasNext) { bldr ++= it.next() } bldr }

then we could use it for all the collection types with appendAll(as, Queue.newBuilder[A]).result()

johnynek · 2022-01-12T19:18:01Z

core/src/main/scala/cats/instances/seq.scala

+      }
+
+      override def combineAllK[A](as: IterableOnce[Seq[A]]): Seq[A] =
+        as.iterator.flatMap(_.iterator).toSeq


same comments as above.

johnynek · 2022-01-12T19:18:09Z

core/src/main/scala/cats/instances/vector.scala

+      }
+
+      override def combineAllK[A](as: IterableOnce[Vector[A]]): Vector[A] =
+        as.iterator.flatMap(_.iterator).toVector


comments above.

TimWSpence · 2022-01-13T11:20:39Z

Sorry @johnynek I was going to check this over before tagging you but you were too quick for me 😂 (hence the error message not being fixed)

… well

TimWSpence · 2022-01-13T14:15:43Z

@johnynek I think I've addressed all your feedback (many thanks!) but do let me know if I've missed something. I also left a few questions for my own edification, hope that's ok!

johnynek

This is great! Thanks

armanbilge

Does Alternative also need a couple more laws to cover fromIterableOnce and fromFoldable? And also can't our instances offer a more optimized implementation of those? E.g. it should be possible to go from an Iterable to a collection without pure on every element.

johnynek · 2022-01-21T23:03:58Z

Yeah. Good point. We could override using appendK/prependK and possibly skip the pure but it has to be per instance because it isn't obvious which is faster.

TimWSpence · 2022-01-27T16:18:20Z

Does Alternative also need a couple more laws to cover fromIterableOnce and fromFoldable? And also can't our instances offer a more optimized implementation of those? E.g. it should be possible to go from an Iterable to a collection without pure on every element.

Thanks! Yeah I thought as I wrote those methods that they should have laws and then didn't get round to it 😅

TimWSpence · 2022-01-28T16:20:43Z

@armanbilge I've added a law for fromIterableOnce that asserts that the implementation matches its default behaviour. I wasn't sure what to do about fromFoldable though - if we added a similar law than it would require a binary incompatible addition of another type parameter on AlternativeLaws for the applicative

armanbilge · 2022-01-28T16:30:55Z

Oh, that's annoying. I wonder if we should make that method final then. Since we can't test other implementations' correctness, seems like we should declare our fromFoldable as the One True Implementation.

TimWSpence · 2022-01-28T16:40:46Z

Oh, that's annoying. I wonder if we should make that method final then. Since we can't test other implementations' correctness, seems like we should declare our fromFoldable as the One True Implementation.

Yep, good call 👍 If an instance has override fromIterableOnce then it should probably be fine anyway

Binary compatibility prohibits us from laws testing it as we would have to add another type parameter to `AlternativeLaws` so we mark it final instead

johnynek · 2022-01-28T16:49:27Z

core/src/main/scala/cats/instances/queue.scala

+
+      override def fromIterableOnce[A](as: IterableOnce[A]): Queue[A] = {
+        val iter = as.iterator
+        val builder = Queue.newBuilder[A]


Won't builder ++= as work here?

I wonder if this is worth the micro optimization of if (as.isEmpty) Queue.empty else { ... } since in that case we don't allocate the builder?

johnynek

added a few minor nits if you want to take them. If you don't want to take them we can possibly address them later.

johnynek · 2022-02-24T19:11:47Z

core/src/main/scala/cats/Alternative.scala

+  def fromIterableOnce[A](as: IterableOnce[A]): F[A] =
+    combineAllK(as.iterator.map(pure(_)))
+
+  final def fromFoldable[G[_]: Foldable, A](as: G[A]): F[A] =


I may have suggested making this final, but on second thought I think it should not be.

It could be that F is very efficient to build from a foldRight or a foldLeft or a foldMap. Since we should assume that the foldable for G knows the most efficient way to do those, the F implementation here may want to call one of those rather than converting to an Iterable[A] and going from there.

This is probably minor, and maybe never a big deal, but the final will block this (I may have suggested or previously agreed with final, if so, apologies for the flip-flop).

Oh yeah, that's a good point.

FTR this was my suggestion in #4084 (comment).

The problem is we can't currently check this implementation in the laws; see #4084 (comment).

So there's a bit of performance vs yolo factor here 😉

This is probably minor, and maybe never a big deal, but the final will block this

We can always take away a final modifier, but we can't add it back. So I'd be fine to leave this until a practical need to change this comes up.

why can't we check the laws? why can't we check that fromFoldable(g) == fromIterable(g.toIterable) in the laws?

Yeah AFAICT we would have to add extra type parameters to AlternativeTests#alternative to do this. Unless I've missed something?

johnynek · 2022-02-24T19:16:42Z

core/src/main/scala/cats/instances/queue.scala

+
+      override def fromIterableOnce[A](as: IterableOnce[A]): Queue[A] = {
+        val iter = as.iterator
+        val builder = Queue.newBuilder[A]


I wonder if this is worth the micro optimization of if (as.isEmpty) Queue.empty else { ... } since in that case we don't allocate the builder?

johnynek · 2022-02-24T19:17:19Z

core/src/main/scala/cats/instances/seq.scala

+        if (iter.isEmpty) None else Some(appendAll(iter, Seq.newBuilder[A]).result())
+      }
+
+      override def fromIterableOnce[A](as: IterableOnce[A]): Seq[A] = {


same pondering about as.isEmpty

AFAICT isEmpty is deprecated in favour of iterator.isEmpty

otherwise this would be a nice optimization

or is allocating an Iterator to maybe not have to allocate a builder preferable?

johnynek · 2022-02-24T19:17:36Z

core/src/main/scala/cats/instances/queue.scala

+
+      override def fromIterableOnce[A](as: IterableOnce[A]): Queue[A] = {
+        val builder = Queue.newBuilder[A]
+        builder ++= as.iterator


do we need .iterator? won't builder ++= as do?

Thanks! Think I got lost in a sea of different versions with different APIs. I've fixed this

johnynek · 2022-02-24T19:18:32Z

core/src/main/scala/cats/instances/seq.scala

@@ -17,6 +20,17 @@ trait SeqInstances extends cats.kernel.instances.SeqInstances {

      def combineK[A](x: Seq[A], y: Seq[A]): Seq[A] = x ++ y

+      override def combineAllOptionK[A](as: IterableOnce[Seq[A]]): Option[Seq[A]] = {
+        val iter = as.iterator
+        if (iter.isEmpty) None else Some(appendAll(iter, Seq.newBuilder[A]).result())


actually, in this pattern, won't if (as.isEmpty) None else Some(appendAll(as.iterator, ...) work also that way we don't need to convert to iterator in the empty case?

Is it possible to check as.isEmpty on an IterableOnce without consuming it?

In fact, in 2.13 IterableOnce#isEmpty is deprecated:

(Since version 2.13.0) Use .iterator.isEmpty instead

I think what we could do here is use IterableOnce#knownSize.

The number of elements in this collection, if it can be cheaply computed, -1 otherwise. Cheaply usually means: Not requiring a collection traversal.

okay. Bummer we have to allocate in order to check emptiness.

Yeah that was what I wanted to write originally but ran into the deprecation

TimWSpence · 2022-03-03T16:27:40Z

added a few minor nits if you want to take them. If you don't want to take them we can possibly address them later.

Many thanks as always for the thorough review! I think I've addressed or replied to all your comments so let me know what you think or if I've missed anything!

johnynek

noticed one minor issue you can take or not (I think it is worth it in theory, but in practice maybe the method never gets called on planet earth).

johnynek · 2022-03-11T18:03:18Z

core/src/main/scala/cats/data/Chain.scala

@@ -1029,6 +1046,7 @@ sealed abstract private[data] class ChainInstances extends ChainInstances1 {

      def empty[A]: Chain[A] = Chain.nil
      def combineK[A](c: Chain[A], c2: Chain[A]): Chain[A] = Chain.concat(c, c2)
+      override def fromIterableOnce[A](xs: IterableOnce[A]): Chain[A] = Chain.fromSeq(xs.iterator.toSeq)


nit...

god... I will never stop finding micro issues:

override def fromIterableOnce[A](xs: IterableOnce[A]): Chain[A] = xs match { case s: Seq[A @unchecked] => // Seq is a subclass of IterableOnce, so the type has to be compatible Chain.fromSeq(s) // pay O(1) not O(N) cost case notSeq => Chain.fromSeq(notSeq.iterator.toSeq) }

Could it make sense to add a Chain.fromIterableOnce that does this instead? I don't really know what to make of these IterableOnce possibly side-effecty things.

sure, we can put it in Chain.

I don't view this as a side effect so much as we are lacking linear or affine types. What we want is to say this method takes ownership of xs. If we had that, then iterating it would be safe (assuming it did no side effect, which we make that assumption all the time in every function and method call).

note: It would be really cool if scala added ownership types but I think it is a bit complex to get right and ergonomic.

TimWSpence · 2022-03-12T14:39:55Z

noticed one minor issue you can take or not (I think it is worth it in theory, but in practice maybe the method never gets called on planet earth).

Thanks! Yeah definitely worth making it O(1) for the more common case of Seq 👍

TimWSpence added 5 commits December 15, 2021 11:13

Add methods from seimgroup/monoid to semigroupk/monoidk

0e0f0d4

Update scaladoc

9ea68b4

More scaladoc

9f348f7

Remove duplicate tests

b0b1404

Fix 2.12 build

72fe714

armanbilge reviewed Dec 15, 2021

View reviewed changes

TimWSpence added 2 commits December 15, 2021 17:02

Fix docs

9fb1bd1

Actually fix docs 🤦

e6fa448

johnynek reviewed Dec 15, 2021

View reviewed changes

Port semigroup/monoid laws to semigroupk/monoidk

c4a620e

TimWSpence dismissed a stale review via c4a620e January 12, 2022 16:19

TimWSpence added 8 commits January 12, 2022 16:34

Cleanup comments

0d7de79

Optimize combineAllK for Option

9264e7c

DRY

5242b6c

Override combineAllK for list

93de0b8

Override combineAllK for Seq

c2d3d4f

Override combineAllK for vector

7099f59

Override combineAllK for Queue

9c07dde

(Hopefully) fix version-specific errors for IterableOnce

47232bf

johnynek reviewed Jan 12, 2022

View reviewed changes

TimWSpence added 6 commits January 13, 2022 11:21

Fix error message

4322e8b

Only require optimizing combineAllOptionK and not combineAllOption as…

05763b2

… well

Avoid creating closure in comineAllOptionK for Option

c237e17

Optimize combineAllOptionK for sequence types

2e5367a

Eliminate unnecessary iterator allocation

bcc07d8

Add Alternative#fromIterableOnce and Alternative#fromFoldable

e350e70

Argh scalafmt

fee0625

johnynek previously approved these changes Jan 19, 2022

View reviewed changes

armanbilge reviewed Jan 21, 2022

View reviewed changes

Law for Alterantive#fromIterableOnce plus optimized implementations

6b5f7fa

TimWSpence dismissed johnynek’s stale review via 6b5f7fa January 28, 2022 16:10

Argh scala 2.12 - why do you do this to me? :(

b531693

TimWSpence added 2 commits January 28, 2022 16:33

Use method not deprecated in 2.12

f5bdeca

More efficient LazyList combineAllOptionK which preserves laziness

7a0cd81

Make Alternative#fromFoldable final

361b6c8

Binary compatibility prohibits us from laws testing it as we would have to add another type parameter to `AlternativeLaws` so we mark it final instead

johnynek reviewed Jan 28, 2022

View reviewed changes

TimWSpence added 3 commits January 28, 2022 17:17

Remove unnecessary Arbitrary instance and fix 2.12 issues

12053e1

Remove unncessary and binary incompatible added implicit param

a02f41e

I really don't think there is a cyclic dependency dotty...

19b3338

TimWSpence dismissed a stale review via 19b3338 February 24, 2022 11:46

johnynek previously approved these changes Feb 24, 2022

View reviewed changes

Avoid unnecessary allocation

549a9f0

TimWSpence dismissed johnynek’s stale review via 549a9f0 March 3, 2022 16:22

johnynek previously approved these changes Mar 11, 2022

View reviewed changes

Optimize fromIterableOnce for Chain

a744378

TimWSpence dismissed johnynek’s stale review via a744378 March 12, 2022 14:30

johnynek approved these changes Mar 12, 2022

View reviewed changes

johnynek merged commit 65e7499 into typelevel:main Mar 12, 2022

SemigroupK/MonoidK methods #4084

SemigroupK/MonoidK methods #4084

Conversation

TimWSpence commented Dec 15, 2021

armanbilge left a comment

Choose a reason for hiding this comment

TimWSpence commented Dec 15, 2021

johnynek left a 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

johnynek commented Dec 15, 2021

TimWSpence commented Dec 18, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimWSpence commented Jan 13, 2022

TimWSpence commented Jan 13, 2022

johnynek left a comment

Choose a reason for hiding this comment

armanbilge left a comment

Choose a reason for hiding this comment

johnynek commented Jan 21, 2022

TimWSpence commented Jan 27, 2022

TimWSpence commented Jan 28, 2022

armanbilge commented Jan 28, 2022

TimWSpence commented Jan 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek left a 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

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

TimWSpence Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

armanbilge Feb 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimWSpence commented Mar 3, 2022

johnynek left a 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

TimWSpence commented Mar 12, 2022

TimWSpence Mar 3, 2022 •

edited

Loading

armanbilge Feb 24, 2022 •

edited

Loading