Lots of documentation + test fixes, updates #579

sritchie · 2016-11-27T22:39:01Z

This PR:

Breaks SucPredLaws.scala out into two files
adds generators and arbitrary instances for DecayedValue, AveragedValue, Moments and AdjoinedUnit[T] to Gen and Arbitrary
Breaks AggregationMonoids.scala out into separate files
Creates Gen.scala with generators for Algebird data structures
Creates Arbitrary.scala with arbitrary instances for Algebird data structures
Moves the generators for Interval, First, Last, Min, Max and ExpHist into these objects
moves the data structures from OrderedSemigroup.scala into their own files
adds full tests for min, max, first, last
adds + operator to min, max, first, last
adds min operator to Min and max operator to Max
adds FirstAggregator and LastAggregator
adds Monoid[Max[Vector[T]]] and Monoid[Max[Stream[T]]]
adds tut-based documentation for first, last, min and max.

sritchie · 2016-11-27T22:46:17Z

This fails binary compatibility... but my preference is to merge anyway and do the appropriate version bump, because the methods that this removes were broken.

sritchie · 2016-11-27T22:46:20Z

cc @johnynek

sritchie · 2016-11-27T23:05:38Z

algebird-test/src/main/scala/com/twitter/algebird/SuccessibleLaws.scala

-   * }
-   *
-   */
-  def predessibleLaws[T: Predecessible: Arbitrary]: Prop = forAll { (t: T, size: Short) =>


this is annoying - this is the ONLY reason this change is binary-incompatible. @isnotinvain, can you check if Twitter is using this anywhere internally?

Let's just call the new one.

sritchie · 2016-11-27T23:06:53Z

Okay, I broke out the real binary incompatibility, the future changes, into #580. This change is still binary-incompatible, but only because of the super-annoying typo, predessibleLaws. I'd bet 100 bucks that no one is using that test, but maybe it's still best to have this force our algebra changes and push out a new binary-incompatible version.

johnynek · 2016-11-27T23:30:24Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+
+  // Zero should have the property that it <= all T
+  def monoid[T](zero: => T)(implicit ord: Ordering[T]): Monoid[Max[T]] =
+    Monoid.from(Max(zero)) { (l, r) => if (ord.gteq(l.get, r.get)) l else r }


Would be nice to override sumOption while we are at it can avoid a lot of reboxing.

johnynek · 2016-11-27T23:32:15Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+  implicit def equiv[T](implicit eq: Equiv[T]): Equiv[Max[T]] = Equiv.by(_.get)
+
+  implicit def semigroup[T](implicit ord: Ordering[T]): Semigroup[Max[T]] =
+    Semigroup.from[Max[T]] { (l, r) => if (ord.gteq(l.get, r.get)) l else r }


Let's do sumOption on this semigroup.

johnynek · 2016-11-27T23:32:43Z

algebird-core/src/main/scala/com/twitter/algebird/Min.scala

+  implicit def equiv[T](implicit eq: Equiv[T]): Equiv[Min[T]] = Equiv.by(_.get)
+
+  // Zero should have the property that it >= all T
+  def monoid[T](zero: => T)(implicit ord: Ordering[T]): Monoid[Min[T]] =


Add sumOption

johnynek · 2016-11-27T23:32:52Z

algebird-core/src/main/scala/com/twitter/algebird/Min.scala

+    Monoid.from(Min(zero)) { (l, r) => if (ord.lteq(l.get, r.get)) l else r }
+
+  implicit def semigroup[T](implicit ord: Ordering[T]): Semigroup[Min[T]] =
+    Semigroup.from[Min[T]] { (l, r) => if (ord.lteq(l.get, r.get)) l else r }


Add sumOption.

johnynek · 2016-11-27T23:33:55Z

algebird-test/src/main/scala/com/twitter/algebird/PredecessibleLaws.scala

+   * property("blah is predecessible") { predecessibleLaws[MyType] }
+   * }}}
+   */
+  def predecessibleLaws[T: Predecessible: Arbitrary]: Prop =


Can you add the old one with the broken name, mark it deprecated and just call this one? Let's not break binary compat if we don't have to.

johnynek · 2016-11-27T23:34:36Z

algebird-test/src/main/scala/com/twitter/algebird/SuccessibleLaws.scala

-   * }
-   *
-   */
-  def predessibleLaws[T: Predecessible: Arbitrary]: Prop = forAll { (t: T, size: Short) =>


Let's just call the new one.

johnynek · 2016-11-27T23:38:47Z

algebird-test/src/test/scala/com/twitter/algebird/DecayedValueSpec.scala

 import org.scalacheck.Prop._

-class DecayedValueLaws extends CheckProperties {
-  import org.scalacheck.Gen.choose
+class DecayedValueSpec extends CheckProperties {


Why change the name of the test?

I was trying to make them all consistent - we have Laws, Spec, Specification, Tests... what do you think of Spec for everything? Laws is fine too, but I'd love them to match.

johnynek · 2016-11-28T00:28:24Z

I don't like Spec personally, but also don't want to open a lot of naming debates on things that exist so my bias would be to leave well enough alone. I particularly hate the WordSpec test style. It is a monstrosity that I hope I never have to see again. I like Lawful things, so Laws is generally good if there are indeed Laws. I think tests and laws are just two different things. It would be nice if laws totally constrained the thing in question, but it usually is not enough so we add some rather ad-hoc tests. We could use laws for code that just encode particular laws and Tests for he actual test fixtures that run the laws. But so many people have their own stylistic preferences I am a bit bearish on such changes because tomorrow someone with a different t style may have a different opinion. Better to make no change without a documented (and ideally CI checked) policy around it.

…

On Sun, Nov 27, 2016 at 14:09 Sam Ritchie ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In algebird-test/src/test/scala/com/twitter/algebird/DecayedValueSpec.scala <#579>: > import org.scalacheck.Prop._ -class DecayedValueLaws extends CheckProperties { - import org.scalacheck.Gen.choose +class DecayedValueSpec extends CheckProperties { I was trying to make them all consistent - we have Laws, Spec, Specification, Tests... what do you think of Spec for everything? Laws is fine too, but I'd love them to match. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#579>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEJdmEkEhzcmcynqd50BktRJr-cwV4Uks5rChvCgaJpZM4K9R9B> .

sritchie

Okay @johnynek, I think that covers all changes!

sritchie · 2016-11-28T00:11:09Z

algebird-test/src/main/scala/com/twitter/algebird/SuccessibleLaws.scala

-   * }
-   *
-   */
-  def predessibleLaws[T: Predecessible: Arbitrary]: Prop = forAll { (t: T, size: Short) =>


sritchie · 2016-11-28T00:34:45Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+  implicit def equiv[T](implicit eq: Equiv[T]): Equiv[Max[T]] = Equiv.by(_.get)
+
+  implicit def semigroup[T](implicit ord: Ordering[T]): Semigroup[Max[T]] =
+    Semigroup.from[Max[T]] { (l, r) => if (ord.gteq(l.get, r.get)) l else r }


sritchie · 2016-11-28T00:34:49Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+
+  // Zero should have the property that it <= all T
+  def monoid[T](zero: => T)(implicit ord: Ordering[T]): Monoid[Max[T]] =
+    Monoid.from(Max(zero)) { (l, r) => if (ord.gteq(l.get, r.get)) l else r }


sritchie · 2016-11-28T00:34:55Z

algebird-core/src/main/scala/com/twitter/algebird/Min.scala

+  implicit def equiv[T](implicit eq: Equiv[T]): Equiv[Min[T]] = Equiv.by(_.get)
+
+  // Zero should have the property that it >= all T
+  def monoid[T](zero: => T)(implicit ord: Ordering[T]): Monoid[Min[T]] =


sritchie · 2016-11-28T00:34:57Z

algebird-core/src/main/scala/com/twitter/algebird/Min.scala

+    Monoid.from(Min(zero)) { (l, r) => if (ord.lteq(l.get, r.get)) l else r }
+
+  implicit def semigroup[T](implicit ord: Ordering[T]): Semigroup[Min[T]] =
+    Semigroup.from[Min[T]] { (l, r) => if (ord.lteq(l.get, r.get)) l else r }


sritchie · 2016-11-28T00:35:05Z

algebird-test/src/main/scala/com/twitter/algebird/PredecessibleLaws.scala

+   * property("blah is predecessible") { predecessibleLaws[MyType] }
+   * }}}
+   */
+  def predecessibleLaws[T: Predecessible: Arbitrary]: Prop =


sritchie · 2016-11-28T00:44:11Z

@johnynek, I changed all the *Spec files I created to *Laws, since they all contain only scalacheck properties. I'll leave old files unchanged from now on, or change to Laws if they in fact only contain scalacheck.

codecov-io · 2016-11-28T02:12:33Z

Current coverage is 65.53% (diff: 91.87%)

Merging #579 into develop will increase coverage by 1.11%

@@            develop       #579   diff @@
==========================================
  Files           111        121    +10   
  Lines          4572       4694   +122   
  Methods        4154       4284   +130   
  Messages          0          0          
  Branches        379        371     -8   
==========================================
+ Hits           2945       3076   +131   
+ Misses         1627       1618     -9   
  Partials          0          0

Powered by Codecov. Last update c91e1fb...0be5b0e

sritchie-stripe · 2016-11-28T18:06:04Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

@@ -88,8 +88,10 @@ object BaseProperties {
    isAssociativeEq[T, T](Equiv[T].equiv _) && semigroupSumWorks[T]

  def commutativeSemigroupLawsEq[T: Semigroup: Arbitrary](eqfn: (T, T) => Boolean) =
-    isAssociativeEq[T, T](eqfn) && isCommutativeEq[T](eqfn)


this was missing semigroupSumWorks[T].

johnynek · 2016-11-28T18:27:54Z

algebird-core/src/main/scala/com/twitter/algebird/First.scala

+
+case class FirstAggregator[T]() extends Aggregator[T, T, T] {
+  def prepare(v: T) = v
+  val semigroup: Semigroup[T] = Semigroup.from { (l: T, r: T) => l }


we almost certainly want the sumOption on this one since we can skip enumerating the iterator in this case (in fact, sumOption is just headOption but for some terrible reason, TraversableOnce[T] does not have that.

johnynek · 2016-11-28T18:28:32Z

algebird-core/src/main/scala/com/twitter/algebird/Last.scala

+
+case class LastAggregator[T]() extends Aggregator[T, T, T] {
+  def prepare(v: T) = v
+  val semigroup: Semigroup[T] = Semigroup.from { (l: T, r: T) => r }


we have to enumerate everything for last, so this is fine without sumOption.

johnynek · 2016-11-28T18:30:01Z

algebird-core/src/main/scala/com/twitter/algebird/First.scala

+}
+
+private[algebird] sealed abstract class FirstInstances {
+  implicit def semigroup[T]: Semigroup[First[T]] = new Semigroup[First[T]] {


we could share this code in object First:

def firstSemigroup[T]: Semigroup[T] = ...

then implicit def semigroup[T]: Semigroup[First[T]] = firstSemigroup[First[T]]

and below you can share the sumOption in the aggregator.

johnynek · 2016-11-28T18:35:55Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+    })
+
+  // TODO: Replace with
+  // scala.collection.mutable.MutableMethods.iteratorCompare when we


this seems like a busted comment. What does cats have to do with scala.collection? I assume this may be referring to some method in cats? Is it in cats.kernel?

yup, busted.

johnynek · 2016-11-28T18:39:01Z

algebird-core/src/main/scala/com/twitter/algebird/Max.scala

+
+case class MaxAggregator[T](implicit ord: Ordering[T]) extends Aggregator[T, T, T] {
+  def prepare(v: T) = v
+  val semigroup = Semigroup.from { (l: T, r: T) => ord.max(l, r) }


should we move this up to being on the companion:

def maxSemigroup[T](implicit ord: Ordering[T]): Semigroup[T] = ...

then call it here? People might want to reuse this.

Also, note this is a Semilattice when we merge algebra we can add that.

johnynek · 2016-11-28T19:16:27Z

algebird-test/src/test/scala/com/twitter/algebird/LastLaws.scala

+  }
+
+  property("Last[Int] is a Semigroup") { semigroupLaws[Last[Int]] }
+  property("Last[String] is a Semigroup") { semigroupLaws[Last[String]] }


don't think we really need two here, for same reason as I mentioned.

johnynek · 2016-11-28T19:17:33Z

algebird-test/src/test/scala/com/twitter/algebird/MaxLaws.scala

+
+  property("Max.{ +, max } works on ints") { maxTest[Int] }
+
+  property("Max should work on non-monoid types like String") { maxTest[String] }


String is a monoid. You mean non-commutative?

good catch - I was using the same tests in MinLaws and MaxLaws. I'll change types. I was looking to exercise a semigroup, since it's defined differently from the monoid

johnynek · 2016-11-28T19:23:07Z

algebird-test/src/test/scala/com/twitter/algebird/MaxLaws.scala

+  }
+
+  property("Max[String] is a commutative semigroup") {
+    commutativeSemigroupLaws[Max[String]]


I think there is a commutativeMonoid here too, right? "" is the zero?

Min[String] is the one that is just a semigroup because there is no max string.

johnynek · 2016-11-28T19:23:42Z

algebird-test/src/test/scala/com/twitter/algebird/MaxLaws.scala

+  }
+
+  property("Max[String] is a commutative monoid") {
+    commutativeMonoidLaws[Max[String]]


this is duplicated above, I think the first should be Min[String]

johnynek · 2016-11-28T19:25:07Z

algebird-test/src/test/scala/com/twitter/algebird/MomentsLaws.scala

-   */
-  def getMoments(xs: List[Double]): Moments =
-    xs.foldLeft(MomentsGroup.zero) { (m, x) => MomentsGroup.plus(m, Moments(x)) }
+object MomentsSpec {


what's going on with this object? Can we have a comment if it is really needed?

nope, deleted, that was waste from a shared function I had then killed. good catch.

johnynek · 2016-11-28T19:30:15Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

+  def isAssociative[T: Semigroup: Arbitrary]: Prop = isAssociativeDifferentTypes[T, T]
+
+  def isAssociativeDifferentTypes[T: Semigroup, U <: T: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


this is just Equiv.universal. Can we use that and deprecate defaultEq?

johnynek · 2016-11-28T19:30:36Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

+
+  // Commutative
+  def isCommutative[T: Semigroup: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


Equiv.universal

johnynek · 2016-11-28T19:31:11Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

-  def semigroupSumWorks[T: Semigroup: Arbitrary: Equiv] = 'semigroupSumWorks |: forAll { (head: T, tail: List[T]) =>
-    Equiv[T].equiv(Semigroup.sumOption(head :: tail).get, tail.foldLeft(head)(Semigroup.plus(_, _)))
+  def semigroupLaws[T: Semigroup: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


Equiv.universal

johnynek · 2016-11-28T19:39:40Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

+    isAssociativeEquiv[T, T] && semigroupSumWorks[T]
+
+  // Commutative Semigroup Laws
+  def commutativeSemigroupLaws[T: Semigroup: Arbitrary]: Prop = {
    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


Equiv.universal

johnynek · 2016-11-28T19:40:39Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

-    val prodZero = !monT.isNonZero(Ring.times(a, b))
-    (Monoid.isNonZero(a) && Monoid.isNonZero(b)) || prodZero
+  def validZero[T: Monoid: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


Equiv.universal

johnynek · 2016-11-28T19:41:37Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

-    val zero = mon.zero
-    eqfn(a, mon.plus(a, zero)) && eqfn(mon.plus(zero, a), a) && eqfn(mon.plus(a, zero), mon.plus(zero, a))
+  def monoidLaws[T: Monoid: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


Equiv.univeral

johnynek · 2016-11-28T19:41:48Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

-  def commutativeMonoidLawsEq[T: Monoid: Arbitrary](eqfn: (T, T) => Boolean) =
-    monoidLawsEq[T](eqfn) && isCommutativeEq[T](eqfn)
+  def commutativeMonoidLaws[T: Monoid: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


Equiv.universal

johnynek · 2016-11-28T19:42:57Z

algebird-test/src/main/scala/com/twitter/algebird/BaseProperties.scala

+    hasAdditiveInversesDifferentTypes[T, T]
+
+  def groupLaws[T: Group: Arbitrary]: Prop = {
+    implicit val eq: Equiv[T] = Equiv.fromFunction(defaultEq)


universal

johnynek · 2016-11-29T17:27:11Z

Looks great, @sritchie Thanks for the hard work on this one!

sritchie-stripe added 8 commits November 24, 2016 13:38

restructure code for first, last, min, max

7d9c73f

abstract exphist instances

eb33436

sumOption for first

837a054

add instances

88eb83f

add equiv

77ef164

add docs

7a57443

add vector and stream

de12da4

more missing

e1cbebb

sritchie added enhancement testing labels Nov 27, 2016

sritchie changed the title ~~More documentation fixes (comes after first/last etc)~~ More documentation fixes (comes after 578) Nov 27, 2016

sritchie mentioned this pull request Nov 27, 2016

Remove invalid Future/Try Group/Ring #548

Closed

next doc update

848a78f

sritchie force-pushed the sritchie/next_doc_round branch from 83de5ec to 848a78f Compare November 27, 2016 23:02

sritchie commented Nov 27, 2016

View reviewed changes

johnynek requested changes Nov 27, 2016

View reviewed changes

oscar fixes

d205e86

sritchie commented Nov 28, 2016

View reviewed changes

sritchie mentioned this pull request Nov 28, 2016

Restructure and document First/Last and Min/Max #578

Closed

change spec to laws

7ee5dc9

sritchie force-pushed the sritchie/next_doc_round branch from c7ae3a5 to 7ee5dc9 Compare November 28, 2016 01:50

remove sumOption override

9f88ff8

sritchie-stripe reviewed Nov 28, 2016

View reviewed changes

fix properties

d2304ac

johnynek requested changes Nov 28, 2016

View reviewed changes

sritchie-stripe added 3 commits November 28, 2016 14:51

oscar fixes

af1b064

IntervalInstances

ee699b4

fix last of oscar's comments

0be5b0e

sritchie mentioned this pull request Nov 29, 2016

Create Equiv versions of all laws, fix broken tests #584

Merged

sritchie changed the title ~~More documentation fixes (comes after 578)~~ Lots of documentation + test fixes, updates Nov 29, 2016

sritchie modified the milestone: 0.13.0 Nov 29, 2016

johnynek approved these changes Nov 29, 2016

View reviewed changes

johnynek merged commit 2c893e3 into develop Nov 29, 2016

johnynek deleted the sritchie/next_doc_round branch November 29, 2016 17:27


		property("Max.{ +, max } works on ints") { maxTest[Int] }

		property("Max should work on non-monoid types like String") { maxTest[String] }

Lots of documentation + test fixes, updates #579

Lots of documentation + test fixes, updates #579

Conversation

sritchie commented Nov 27, 2016 • edited Loading

sritchie commented Nov 27, 2016

sritchie commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sritchie commented Nov 27, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Nov 28, 2016 via email

sritchie 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

sritchie commented Nov 28, 2016

codecov-io commented Nov 28, 2016 • edited Loading

Current coverage is 65.53% (diff: 91.87%)

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Nov 29, 2016

sritchie commented Nov 27, 2016 •

edited

Loading

codecov-io commented Nov 28, 2016 •

edited

Loading