Skip to content

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

Open
cassava wants to merge 1 commit into
HdrHistogram:masterfrom
cassava:master
Open

Make it harder for Mean() and StdDev() to overflow#21
cassava wants to merge 1 commit into
HdrHistogram:masterfrom
cassava:master

Conversation

@cassava

@cassava cassava commented Aug 24, 2016

Copy link
Copy Markdown

This is my proposal for solving issue #20

@codahale

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

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

@filipecosta90

Copy link
Copy Markdown
Collaborator

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

@ahothan

ahothan commented Nov 24, 2020

Copy link
Copy Markdown
Collaborator

@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.

@dkropachev

Copy link
Copy Markdown
Contributor

@filipecosta90 , can you please take a look at it, looks very reasonable.

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

5 participants