[query] Improve precision for variance and stddev of equal values #2799
Conversation
| # FAILING issue #10. (it is almost zero) | ||
| #eval instant at 50m stddev(http_requests) | ||
| # {} 0.0 | ||
| eval instant at 50m stddev(http_requests) | ||
| {} 0.0 | ||
|
|
||
| # FAILING issue #11. (it is almost zero) | ||
| #eval instant at 50m stdvar(http_requests) | ||
| # {} 0.0 | ||
| eval instant at 50m stdvar(http_requests) | ||
| {} 0.0 |
There was a problem hiding this comment.
While the previous implementation were returning not 0.0 for these tests, the tests were not failing since the results were inside the tolerance range (see code snippet below). Hence, I added a couple of tests in function_test.go file.
m3/src/query/test/compatibility/test.go
Line 70 in d47dcc3
There was a problem hiding this comment.
This is unexpected. If this was not failing, we would not have noticed the issue.
Please check, maybe the tolerance was added with some later commit (after these tests were commented out)?
If this tolerance is no longer needed, we might want to abandon it.
There was a problem hiding this comment.
It was failing in the initial commit (2c4f65c), but stopped after this line change (440171d#diff-25ae4a7d6eea65e4db2787e6a61f5aea2646020856df3c3f5c7a5cb6b368be1cL563-R565).
I guess the tolerance is still needed, though we could try tuning a bit more how the 0.0 vs almost-zero values are compared.
An idea: maybe it would be useful to test both successes and failures? E.g. instead of commenting tests out, we could introduce a failing flag (e.g. eval_fail or whatever is easier to implement) and check in TestEvaluations that these tests are actually failing. That way we could see when some changes fix (or un-fail) not only the intended tests, but some others, too.
There was a problem hiding this comment.
After discussing with @linasm we've decided to update this line 5db10aa#diff-25ae4a7d6eea65e4db2787e6a61f5aea2646020856df3c3f5c7a5cb6b368be1cL565-R565
| // Using Welford's online algorithm for calculating variance | ||
| // https://en.wikipedia.org/wiki/Algorithms_for_calculating_variance#Welford's_online_algorithm | ||
| // | ||
| // This algorithm is used in Prometheus and also should provide better numerical precision than |
There was a problem hiding this comment.
| { | ||
| "five 1.33e-5", | ||
| []float64{1.33e-5, 1.33e-5, 1.33e-5, 1.33e-5, 1.33e-5}, | ||
| }, | ||
| { | ||
| "three 13.3", | ||
| []float64{13.3, 13.3, 13.3}, | ||
| }, |
There was a problem hiding this comment.
13.3 is from the aggregators.test file. I've randomly tried a handful of values, 1.33e-5 is one that makes the previous version of varianceFn()/stddevFn() fail the test.
| count := 0 | ||
| partialMean := 0.0 | ||
| partialVarTimesCount := 0.0 // for better precision, calculate `variance * count` and divide at the end |
There was a problem hiding this comment.
Nit: consider putting these into a var (...) block. Usually it looks cleaner that way.
| @@ -562,7 +562,7 @@ func almostEqual(a, b float64) bool { | |||
| diff := math.Abs(a - b) | |||
|
|
|||
| if a == 0 || b == 0 || diff < minNormal { | |||
| return diff < epsilon | |||
| return diff < epsilon*minNormal | |||
There was a problem hiding this comment.
This seems a bit weird, thought that minNormal is the lowest possible float, although guess this is what Prom does in their tests too 🤷♂️
There was a problem hiding this comment.
TIL: min-normal is not the lowest one, a nice description in here. Seems that when you go below that, you start losing precision (number of digits) and there might be a performance penalty.
* master: [query] Improve precision for variance and stddev of equal values (#2799) Support dynamic namespace resolution for embedded coordinators (#2815) [dbnode] Add index regexp DFA compilation cache to avoid allocating DFAs for same expressions (#2814) [dbnode] Set default cache on retrieve to false, prepare testing with cache none (#2813) [tools] Output annotations as base64 (#2743) Add Reset Transformation (#2794) [large-tiles] Fix for a reverse index when querying downsampled namespace (#2808) [dbnode] Evict info files cache before index bootstrap in peers bootstrapper (#2802) [dbnode] Bump default filesystem persist rate limit (#2806) [coordinator] Add metrics and configurability of downsample and ingest writer worker pool (#2797) [dbnode] Fix concurrency granularity of seekerManager.UpdateOpenLease (#2790) [dbnode] Use correct import path for atomic library (#2801) Regenerate carbon automapping rules on namespaces changes (#2793) Enforce matching retention and index block size (#2783) [query] Add ClusterNamespacesWatcher and use to generate downsample automapper rules (#2782) [query][dbnode] Wire up etcd-backed Clusters implementation in coordinator (#2758) [query] Fix quantile() argument not being passed through (#2780) [query] Add "median" aggregation to Graphite aggregate() function (#2774) [r2] Ensure KeepOriginal is propagated when adding/reviving rules (#2796) [dbnode] Update default db read limits (#2784)
What this PR does / why we need it:
Improves precision for
variance()andstddev()aggregation operations when the vector contains equal values. In some cases, this caused in mismatch between Prometheus and M3, where the former would return0.0and the latter would return a number that is close to zero, but not equal.Special notes for your reviewer:
Using the same algorithm as in Prometheus
Does this PR introduce a user-facing and/or backwards incompatible change?:
Does this PR require updating code package or user-facing documentation?: