Mutable MomentsState, more efficient + for double added to Moments #1050

sritchie · 2022-01-13T04:48:32Z

This PR:

fixes an issue with build.sbt preventing my local jdk17 from compiling
adds + methods to Moments allowing addition with other Moments or doubles
adds MomentsState for efficient mutable accumulation of a Moments instance from iterables of either doubles or other moments instances

MomentsState pulls the former mutable implementation from sumOption and adds a second one for mutably adding in new doubles.

sritchie · 2022-01-13T04:49:16Z

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

+    * Returns a [[Fold]] instance that uses `+` to accumulate deltas into this
+    * [[Moments]] instance.
+    */
+  def fold: Fold[Double, Moments] =


This is the style we used in ExpHist, where the fold is an instance method that lets you fold into THIS Moments, implemented by

_ => Moments.MomentsState.fromMoments(this)

Let me know if you want to just have an empty fold on Moments.

Huh... looking at it now, it seems a bit strange to me, but I can see the benefit of being able to start with a non empty value to resume some updates.

sritchie · 2022-01-13T04:49:33Z

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

+        val delta_n = delta / countCombined
+        val delta_n2 = delta_n * delta_n
+        val delta_n3 = delta_n2 * delta_n
+        val count_sq = count * count


I also killed the math.pow calls everywhere.

sritchie · 2022-01-13T04:49:45Z

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

@@ -111,6 +245,8 @@ object Moments {

  val aggregator: MomentsAggregator.type = MomentsAggregator

+  val fold: Fold[Double, Moments] = momentsMonoid.zero.fold


here we go, the zero fold.

sritchie · 2022-01-13T04:49:57Z

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

      val iter = items.toIterator
-


this implementation lives in MomentsState now.

sritchie · 2022-01-13T04:50:16Z

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

@@ -100,7 +132,7 @@ class MomentsTest extends AnyWordSpec with Matchers {
   * Given a list of doubles, create a Moments object to hold the list's central moments.
   */
  def getMoments(xs: List[Double]): Moments =
-    MomentsAggregator(xs)
+    Moments.aggregator(xs)


I switched this so we get coverage on the aggregator line :)

sritchie · 2022-01-13T04:50:23Z

build.sbt

@@ -69,7 +69,7 @@ val sharedSettings = Seq(
        Nil
    }
  },
-  javacOptions ++= Seq("-target", "1.6", "-source", "1.6"),
+  javacOptions ++= Seq("-target", "1.8", "-source", "1.8"),


here is the build change.

codecov-commenter · 2022-01-13T05:32:51Z

Codecov Report

Merging #1050 (8232a9a) into develop (d96093b) will increase coverage by 0.00%.
The diff coverage is 87.01%.

@@           Coverage Diff            @@
##           develop    #1050   +/-   ##
========================================
  Coverage    89.71%   89.71%           
========================================
  Files          124      124           
  Lines         9914     9957   +43     
  Branches       497      501    +4     
========================================
+ Hits          8894     8933   +39     
- Misses        1020     1024    +4

Impacted Files	Coverage Δ
...main/scala/com/twitter/algebird/MomentsGroup.scala	`81.33% <87.01%> (+1.89%)`	⬆️
.../main/scala/com/twitter/algebird/Applicative.scala	`51.72% <0.00%> (-3.45%)`	⬇️
...src/main/scala/com/twitter/algebird/Interval.scala	`84.34% <0.00%> (-0.87%)`	⬇️
.../main/scala/com/twitter/algebird/BloomFilter.scala	`95.13% <0.00%> (-0.45%)`	⬇️
.../scala/com/twitter/algebird/immutable/BitSet.scala	`97.21% <0.00%> (+0.42%)`	⬆️
.../main/scala/com/twitter/algebird/Successible.scala	`91.66% <0.00%> (+8.33%)`	⬆️
...main/scala-2.12-/com/twitter/algebird/compat.scala	`83.33% <0.00%> (+16.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d96093b...8232a9a. Read the comment docs.

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

johnynek · 2022-01-13T19:35:20Z

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

+    * Returns a [[Fold]] instance that uses `+` to accumulate deltas into this
+    * [[Moments]] instance.
+    */
+  def fold: Fold[Double, Moments] =


Huh... looking at it now, it seems a bit strange to me, but I can see the benefit of being able to start with a non empty value to resume some updates.

johnynek · 2022-01-13T19:39:55Z

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

+       * but we do it to avoid allocating a new Moments on every item in the
+       * loop. the Monoid laws test that sum matches looping on plus
+       */
+      val countCombined = count + b.m0D


I guess we could use a variant of Kahan summation on countCombined, right

Yup, and if we had the fold version, or KahanState, it would fit nicely here.

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

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

sritchie · 2022-01-13T20:39:22Z

Okay, fixed up @johnynek !

sritchie · 2022-01-13T21:44:34Z

Benchmark results... I'm a little surprised at the winner here.

[info] Benchmark                         (numElements)   Mode  Cnt    Score   Error  Units
[info] MomentsBenchmark.timeAggregate            10000  thrpt    3   7968.580 ±  625.872  ops/s
[info] MomentsBenchmark.timeFold                 10000  thrpt    3  12852.096 ±  760.259  ops/s
[info] MomentsBenchmark.timePlusDoubles          10000  thrpt    3  15150.855 ± 743.752  ops/s
[info] MomentsBenchmark.timePlusMoments          10000  thrpt    3  13478.708 ± 675.039  ops/s
[info] MomentsBenchmark.timeSumOption            10000  thrpt    3  13576.123 ± 1111.999  ops/s

Wow, and here are the benchmarks that could run BEFORE this PR and #1049. Optimizing plus really matters!!

[info] Benchmark                         (numElements)   Mode  Cnt    Score   Error  Units
[info] MomentsBenchmark.timeAggregate            10000  thrpt    3  227.477 ± 2.977  ops/s
[info] MomentsBenchmark.timePlusMoments          10000  thrpt    3  223.974 ± 7.554  ops/s
[info] MomentsBenchmark.timeSumOption            10000  thrpt    3  226.067 ± 3.714  ops/s

new moments impls

bf1a2dc

sritchie requested a review from johnynek January 13, 2022 04:48

sritchie commented Jan 13, 2022

View reviewed changes

sritchie added 5 commits January 12, 2022 21:51

formatting

b9924c7

test

f083913

maybe fix

31ce810

fixes

c2222b4

scaladoc

61376a4

sritchie mentioned this pull request Jan 13, 2022

More efficient computation in MomentsMonoid #1049

Merged

scalafmt

faef619

johnynek reviewed Jan 13, 2022

View reviewed changes

algebird-core/src/main/scala/com/twitter/algebird/MomentsGroup.scala Show resolved Hide resolved

johnynek reviewed Jan 13, 2022

View reviewed changes

algebird-test/src/test/scala/com/twitter/algebird/MomentsLaws.scala Outdated Show resolved Hide resolved

respond

656c8aa

sritchie added 2 commits January 13, 2022 14:02

fix

e4403cb

add benchmark

dfeafa1

bench with foldLeft vs reduce

8232a9a

johnynek approved these changes Jan 13, 2022

View reviewed changes

sritchie merged commit a84caa0 into develop Jan 13, 2022

sritchie deleted the sritchie/moments_update branch January 13, 2022 23:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mutable MomentsState, more efficient + for double added to Moments #1050

Mutable MomentsState, more efficient + for double added to Moments #1050

sritchie commented Jan 13, 2022

sritchie Jan 13, 2022 •

edited

Loading

johnynek Jan 13, 2022

sritchie Jan 13, 2022

sritchie Jan 13, 2022

sritchie Jan 13, 2022

sritchie Jan 13, 2022

sritchie Jan 13, 2022

codecov-commenter commented Jan 13, 2022 •

edited

Loading

johnynek Jan 13, 2022

johnynek Jan 13, 2022

sritchie Jan 13, 2022

sritchie commented Jan 13, 2022

sritchie commented Jan 13, 2022 •

edited

Loading

		@@ -111,6 +245,8 @@ object Moments {

		val aggregator: MomentsAggregator.type = MomentsAggregator

		val fold: Fold[Double, Moments] = momentsMonoid.zero.fold

Mutable MomentsState, more efficient + for double added to Moments #1050

Mutable MomentsState, more efficient + for double added to Moments #1050

Conversation

sritchie commented Jan 13, 2022

sritchie Jan 13, 2022 • edited Loading

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

codecov-commenter commented Jan 13, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sritchie commented Jan 13, 2022

sritchie commented Jan 13, 2022 • edited Loading

sritchie Jan 13, 2022 •

edited

Loading

codecov-commenter commented Jan 13, 2022 •

edited

Loading

sritchie commented Jan 13, 2022 •

edited

Loading