Monoid[Comparison] - lexographical ordering

[adelbertc]

No description provided.

[adelbertc]

Binary compat check failed, I assume because I renamed the instance. Should I add it under a separate member or do we want to let this one slide in?

[LukaJCB]

LGTM, thanks! :)

[ceedubs]

Is there anything in particular that motivates this Monoid instance? It seems somewhat arbitrary to me, and I'm curious about how one would use it. For example I wouldn't really expect that LessThan |+| GreaterThan is LessThan but GreaterThan |+| LessThan is GreaterThan.

On a tangential note something that seems a little more straightforward to me would be strengthening from Eq[Comparison] to Order[Comparison], but I don't really have a use-case for that.

[adelbertc]

@ceedubs It implements something analogous to lexographical ordering - if you want to compare two A's and have a bunch of functions (A, A) => Comparison this instance describes "compare with f, and if those are equal then compare with g, and if those are equal then compare with ..."

Most recently I used this here (which is what prompted this PR), and I've used the instance in Scalaz several times in the past when I want behavior described above.

As a fun fact, Order#whenEqual is analogous to this, but a bit strange since it implies multiple type class instances which is sketchy ;)

def whenEqual(o: Order[A]): Order[A]

Returns a new Order[A] instance that first compares by the original Order instance and uses the provided Order instance to "break ties".

[ceedubs]

Thanks @adelbertc I can see why you would want that.

I apologize in advance, but I'm going to take this down a bit of a rabbit hole.

I've done this sort of thing with scalaz a lot where I do something like:

  implicit val posOrder: Order[Pos] =
    Order.by(_.fileName) |+| Order.by(_.line) |+| Order.by(_.column)

I added a Monoid[Order[A]] instance here back when both Monoid and Order were defined in algebra. At the time, @johnynek suggested not making it implicit, and I was fine with that because I could just change |+| to whenEqual in the code above. However, it looks like the whenEqual method on the Order trait was removed in #1997.

To me something about the Monoid[Order[A]] instance seems more straightforward than the Monoid[Comparison] instance, but I can't really justify that sentiment, and it's pretty arbitrary. However, I can't really see a justification for having an implicit Monoid[Comparison] instance and not an implicit Monoid[Order] instance.

Furthermore, I think that chaining together Order instances like in my sample above is currently harder with Cats than it should be. I wonder if @johnynek still feels the same way about the Monoid instance for Order not being implicit? To me this seems like the most likely Monoid[Order[A]] instance that someone might be looking for, and we already are kind of opinionated about Monoid[Int], Monoid[Option[A]], etc.

Sorry for the wall of text. I just kind of feel like this might mostly be useful as a workaround for something that should probably be easier anyway, and it seemed a little inconsistent to add this. I'm definitely curious to hear people`s thoughts.

[adelbertc]

One important difference between doing this through Order[A] versus Comparison is Order is a proper type class, which makes it difficult (dare I say wrong!) to change it on an ad-hoc basis. One can imagine a product type with several fields..

final case class Foo(
  a: Int,
  b: String,
  c: Double,
  d: List[Int]
  /* ... *./
)

and in different contexts only want to compare between a, c, and d lexicographically, in other contexts maybe just b and d, etc. I understand how this seems like a workaround, but I think there's nothing bad happening here, you're just doing ad-hoc comparison of two values based on functions, and different contexts will care about different functions.

[ceedubs]

@adelbertc you raise some good points. Personally I've kind of settled on a middle-ground when it comes to type class coherence. I'm fine with there being multiple Order[Foo] instances, but I don't think that you should ever make more than one of them implicit.

👍 I'm good with the Monoid[Comparison] instance. My main concern would be the binary incompatibility within kernel. We could probably get around it by either leaving the instance with a name that doesn't describe it super well or adding a separate instance (the former sounds better than the latter to me, but if we are okay with a binary incompatibility here right before 1.0 comes out, that's probably preferred). Maybe @johnynek wants to weigh in?

[rossabaker]

I ran into similar while upgrading from cats-0.9 last night. I settled on:

    Order.whenEqualMonoid.combineAll(List(
      Order.by(_.vertex),
      Order.by(_.label),
      Order.by(_.inAdj.sorted),
      Order.by(_.outAdj.sorted)))

I like this a little better than the Monoid[Comparison], because I don't have to repeat the function on the comparands, as in @adelbertc's ermine-parser example. But I did miss the whenEqual from 0.9 and did half expect to find an implicit monoid to replace it with |+|.

[ceedubs]

@rossabaker Thanks for weighing in. That looks like a pretty clean solution.

I may separately pursue an agenda of an implicit Monoid[Order[A]] instance, but I think that @adelbertc has at least offered explanations of why this Monoid[Comparison] instance is useful and why it might make sense to make it implicit even if the Monoid[Order[A]] instance isn't implcit. I don't want to slow down his PR with my separate agenda :)

[adelbertc]

Does having one preclude having the other? Are there other more appropriate instances of a Monoid[Comparison]?

[ceedubs]

Does having one preclude having the other?

No, I definitely don't think so.

Are there other more appropriate instances of a Monoid[Comparison]?

I doubt it. It seems unlikely to me that someone would reach for one outside of the use-case that you described.

[adelbertc]

I think for this PR 2 questions remain:

1. Do we want this instance?

2. If (1) is "yes" then what to do about the binary compat. failure - do we let it slide? Do we keep the old name and just add the new instance under a different name?

[SystemFw]

ah, I thought it was just me finding Order a bit cumbersome :)
I ended up doing similar to @rossabaker (combine/combineAll rather than making an implicit and using |+|).
Another imho weird thing is having reverse be a function in the Order companion object, rather than a method on Order.

[ceedubs]

This has been sitting around for a while, but FWIW I vote for adding this and for the time being maintaining the current name to preserve binary compatibility (but we may want to change the name for Cats 3.0).

[larsrh]

#3753