Add scale method to Moments #850
Conversation
There was a problem hiding this comment.
Another approach that might be worth exploring: declare a new version of moments where count is a Double instead of a Long, with a trait that implements all of the shared business logic between both versions in terms of a Numeric (or whatever the best mechanism is for making this generic). Overriding methods like equals is just kind of a lot here.
| else | ||
| /* don't return junk when the moment is not defined */ | ||
| Double.NaN | ||
| math.sqrt(m0D) * m3 / math.pow(m2, 1.5) |
There was a problem hiding this comment.
No longer filtering on count > 2 is a semantic change that's probably worth at least calling out in the release notes or something.
There was a problem hiding this comment.
indeed it is. I will call it out, but since we returned NaN before, we were already in a "silent-ish" failure more.
|
Yeah. I could just copy all the code and leave the old one for binary compatibility but I don't really want to have to two copies in the repository. The mima check shows we needed to make all these changes to support binary compatibility. It really wasn't that much work to implement everything. |
Codecov Report
@@ Coverage Diff @@
## develop #850 +/- ##
===========================================
+ Coverage 89.29% 89.34% +0.04%
===========================================
Files 117 117
Lines 9326 9348 +22
Branches 365 371 +6
===========================================
+ Hits 8328 8352 +24
+ Misses 998 996 -2
Continue to review full report at Codecov.
|
|
I don't doubt that this was straightforward to write, but it's pretty difficult to read and I suspect maintain. Ultimately, algebird is your baby more so than anyone else's, and IMO you've earned the right to make these sorts of aesthetic decisions for the library. But my 2¢ is that this would all be much cleaner by just defining a |
algebird-core/src/main/scala/com/twitter/algebird/MomentsGroup.scala
Outdated
Show resolved
Hide resolved
| @@ -94,6 +188,8 @@ class MomentsMonoid extends Monoid[Moments] with CommutativeMonoid[Moments] { | |||
| * Uses a more stable online algorithm which should be suitable for | |||
| * large numbers of records similar to: | |||
| * http://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Parallel_algorithm | |||
| * | |||
| * we no longer use this, but we can't remove it due to binary compatibility | |||
| */ | |||
| def getCombinedMean(n: Long, an: Double, k: Long, ak: Double): Double = | |||
There was a problem hiding this comment.
should we add a @deprecated as well?
….scala Co-authored-by: Filipe Regadas <[email protected]>
|
|
|
@regadas merged your suggestion, sorry for being slow. I merged develop and lets make sure CI is green, then it would be nice to merge. |
johnynek
force-pushed
the
oscar/add_moments_scale
branch
from
ad0ca48 to
6bc4189
Compare
|
@johnynek awesome! yeah don't worry about it! |
This is a follow up to #829 which adds the scale method to Moments.
I had thought we would have to wait to break binary compatibility, but by writing the case class methods by hand, we can actually make the internal state a double and add the scale method to Moments.
This offers the ability to do exponentially decayed moments, which can be useful to aggregate over long time scales but not have very old values dominate the results.
It's a bit ugly to write this code out by hand, but binary compatibility is worth it, IMO.
PTAL @eigenvariable @regadas