Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it harder for Mean() and StdDev() to overflow #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cassava
Copy link

@cassava cassava commented Aug 24, 2016

This is my proposal for solving issue #20

@codahale
Copy link
Contributor

This looks great, but the admittedly very touchy tests don’t pass. Feel free to round the stddev and I’ll gladly merge this. Thanks for the patch!

@filipecosta90
Copy link
Collaborator

bumping this @cassava .
The tests are still flaky. How do you want to proceed with it? :

:~/go/src/github.com/HdrHistogram/hdrhistogram-go$ make test
GO111MODULE=on go get -t -v ./...
GO111MODULE=on go fmt ./...
GO111MODULE=on go test -count=1 ./...
--- FAIL: TestStdDev (0.01s)
    hdr_test.go:80: StdDev was 288675.1403682712, but expected 288675.1403682715
FAIL
FAIL    github.com/HdrHistogram/hdrhistogram-go 0.153s
FAIL
make: *** [Makefile:35: test] Error 1

@filipecosta90
Copy link
Collaborator

@ahothan we can approve this assuming we merge #40 first.

@filipecosta90
Copy link
Collaborator

@ahothan I've confirmed that all tests pass with the current master. If we want we can merge it. wdyt?

@ahothan
Copy link
Collaborator

ahothan commented Nov 24, 2020

@filipecosta90
I was wondering if this could introduces the same kind of issue at the other end of the spectrum of possible values, when total count is extremely high and values are extremely small, but it looks like float64 can handle that in all realistic cases as the smallest number for float64 is ~10**(-308)
So this patch looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Overflow while calculating mean
4 participants