make stddev work #2354
make stddev work #2354
Conversation
|
i expect this PR will fail as it is (i have an extra commit that should fix it) but this seems like a test script failure |
|
@neonstalwart agreed, that's a weird one. kicking the build to see if it pulls down correctly the next time. |
|
@toddboom 😞 didn't work |
|
Build script has been fixed, please rebase so CI testing can re-run. |
neonstalwart
force-pushed
the
stddev
branch
4 times, most recently
from
118095c to
3b8b526
Compare
|
i need some help here... i'm fairly sure that |
|
hmm... i've got this working for TestSingleServer but it fails for Test3NodeServer. anyone got an idea? |
|
Thanks @neonstalwart -- it is a failure unrelated to your change. We are currently investigating the root cause of these failures in CI. |
this includes avoiding overflow for large values
|
roll the 🎲 again and this time it passes 🎉 |
|
this is ready for review now - once the values were properly unmarshalled everything started to make more sense. |
|
i'm not sure who is responsible for which parts of influxdb but it looks like @pauldix shows up a lot in the blame for influxql/functions.go so he gets a mention to see if this could get a review. |
| @@ -419,7 +425,7 @@ func ReduceStddev(values []interface{}) interface{} { | |||
| sq := math.Pow(dif, 2) | |||
| variance += sq | |||
| } | |||
| variance = variance / float64(len(data)-1) | |||
| variance = variance / float64(count-1) | |||
There was a problem hiding this comment.
Should check for count == 1 or this'll be bad
There was a problem hiding this comment.
look at line 410
There was a problem hiding this comment.
it probably wouldn't be a bad idea to add a test for count < 2 though. i can work on that if you want.
There was a problem hiding this comment.
ah, no, that's fine, didn't see it.
|
+1 |
|
i went ahead and added a test for one point - i think it's a significant case. |
|
Thanks @neonstalwart! |
it looks like stddev isn't working but there are no tests covering it