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.

Sign up for GitHub

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 {C,c}omparison to Order, fixes #1101 #1102

Merged
merged 3 commits into from
Jun 9, 2016
Merged

Add {C,c}omparison to Order, fixes #1101 #1102

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8bf39d
Add {C,c}omparison to Order, fixes #1101
adelbertc Jun 9, 2016
753b5a3
Add Comparison.fromInt, make toInt a val, add PartialOrder comparison
adelbertc Jun 9, 2016
50da459
Fix implicit numeric widening in 2.10.6
adelbertc Jun 9, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 22 additions & 1 deletion kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import cats.kernel.std.all._

import org.typelevel.discipline.{ Laws }
import org.typelevel.discipline.scalatest.Discipline
import org.scalacheck.{ Arbitrary }
import org.scalacheck.{ Arbitrary, Gen }
import Arbitrary.arbitrary
import org.scalatest.FunSuite
import scala.util.Random
Expand Down Expand Up @@ -79,6 +79,27 @@ class LawTests extends FunSuite with Discipline {
laws[GroupLaws, (Int, Int)].check(_.band)

laws[GroupLaws, Unit].check(_.boundedSemilattice)

// Comparison related
implicit val arbitraryComparison: Arbitrary[Comparison] =
Arbitrary(Gen.oneOf(Comparison.GreaterThan, Comparison.EqualTo, Comparison.LessThan))

laws[OrderLaws, Comparison].check(_.eqv)

test("comparison") {
val order = Order[Int]
val eqv = Eq[Comparison]
eqv.eqv(order.comparison(1, 0), Comparison.GreaterThan) &&
eqv.eqv(order.comparison(0, 0), Comparison.EqualTo) &&
eqv.eqv(order.comparison(-1, 0), Comparison.LessThan)
}

test("signum . toInt . comparison = signum . compare") {
check { (i: Int, j: Int) =>
Eq[Int].eqv(Order[Int].comparison(i, j).toInt.signum, Order[Int].compare(i, j).signum)
}
}

// esoteric machinery follows...

implicit lazy val band: Band[(Int, Int)] =
Expand Down
19 changes: 19 additions & 0 deletions kernel/src/main/scala/cats/kernel/Comparison.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package cats.kernel

/** ADT encoding the possible results of a comparison */
sealed abstract class Comparison extends Product with Serializable {
/** The signum of this comparison */
def toInt: Int = this match {
case Comparison.GreaterThan => 1
case Comparison.EqualTo => 0
case Comparison.LessThan => -1
}
Copy link
Contributor

Choose a reason for hiding this comment

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

out of curiosity, @non, do you know it the above is faster or:

sealed abstract class Comparison(val toInt)
case object GreaterThan extends Comparison(1)
case object EqualTo extends Comparison(0)
case object LessThan extends Comparison(-1)

It seems to me the above would be.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I think having Comparison instances "know" their integer value (either through fields or through methods on the subtype) would be better than using pattern matching in .toInt.

Copy link
Contributor

Choose a reason for hiding this comment

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

It may be different with a val like that, but fwiw, at one point I benchmarked having scalaz's Maybe implement fold via pattern-matching in the parent vs dynamic dispatch to the child implementation and the former seemed to have a slight edge. It may be worth throwing together a benchmark if this is something that we care about. (I'm okay with it not being something that we care about, since people can always use the Int version of compare if they really want performance.)

}

object Comparison {
final case object GreaterThan extends Comparison
final case object EqualTo extends Comparison
final case object LessThan extends Comparison
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be nice to have a fromInt method.


implicit val catsKernelEqForComparison: Eq[Comparison] = Eq.fromUniversalEquals
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably make this an Order as opposed to just an Eq. I don't know if it would ever really be used that way in practice, though.

}
11 changes: 11 additions & 0 deletions kernel/src/main/scala/cats/kernel/Order.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@ trait Order[@sp A] extends Any with PartialOrder[A] { self =>
*/
def compare(x: A, y: A): Int

/**
* Like `compare`, but returns a [[cats.kernel.Comparison]] instead of an Int.
* Has the benefit of being able to pattern match on, but not as performant.
*/
def comparison(x: A, y: A): Comparison = {
val r = compare(x, y)
if (r > 0) Comparison.GreaterThan
else if (r == 0) Comparison.EqualTo
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it OK we're using == instead of Eq[Int] ? I figure it's more performant but maybe not?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, when working with a known primitive (e.g. Int) using == is correct.

Copy link
Contributor

@non non Jun 9, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with @ceedubs that this should probably use Comparison.fromInt(compare(x, y)) (and that the code here should be moved to Comparison).

else Comparison.LessThan
}

def partialCompare(x: A, y: A): Double = compare(x, y).toDouble

/**
Expand Down