Skip to content

Fix failing tests due to precision errors on arm64 #58

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

Merged
merged 4 commits into from
Jan 13, 2021

Conversation

Gu1nness
Copy link
Contributor

@Gu1nness Gu1nness commented Dec 15, 2020

Co-authored-by: @nbraud
Fixes #33

Co-authored-by: nicoo <nicoo@debian.org>
Bug-Debian: https://bugs.debian.org/976562
Bug: montanaflynn#33
Forwarded: no
@nicoonoclaste
Copy link

FYI, this can be tested without access to a non-amd64 machine, using qemu-user-static.
Just GOARCH=arm64 go test will work, thanks to some binfmt magic.

@coveralls
Copy link

coveralls commented Dec 15, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 2c61242 on Gu1nness:fix-tests-outside-amd64 into ed2e831 on montanaflynn:master.

@anthonyfok
Copy link

Thank you so much @Gu1nness and @nbraud! This is an awesome fix to #33!

There is only one tiny thing I would recommend, and that is to run go fmt and push to this pull request again, and that would unify the spacing for correlation_test.go, test_utils_test.go, util.go.

Many thanks!

@nicoonoclaste
Copy link

@anthonyfok I was hoping you'd have a suggestion for a better place to put test utilities than test_utils_test ;3

@anthonyfok
Copy link

@nbraud Ha ha! I didn't look too closely at the rest of the patch / pull request yet, but in my limited (and abandoned) experiment, I named it tolerance_test.go so it matches the main function tolerance() therein, but with the disadvantage that it looks too much like one of the other features (e.g. variance.go and variance_test.go), so your naming of test_utils_test.go is good that it is unambiguous. ^_^

@@ -8,12 +8,13 @@ import (
"github.com/montanaflynn/stats"
)

func ExampleCorrelation() {
func ExampleCorrelation(l *testing.T) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fails for older Go versions which won't accept an argument in an en Example test:

https://travis-ci.org/github/montanaflynn/stats/builds/749863684

s1 := []float64{1, 2, 3, 4, 5}
s2 := []float64{1, 2, 3, 5, 6}
a, _ := stats.Correlation(s1, s2)
fmt.Println(a)
// Output: 0.9912407071619302
if !veryclose(a, 0.9912407071619302) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could use stats.Round here with the same result and added benefit of showing off another exported function instead of a private function that cannot be copy / pasted as one would expect from an example.

@montanaflynn
Copy link
Owner

Great work @Gu1nness and @nbraud, happy to see this solved!

I see it failed tests for older Go versions: https://travis-ci.org/github/montanaflynn/stats/builds/749863684 and left some comments on how I think we could fix that and improve the example at the same time.

I also agree with @anthonyfok that running go fmt to fix the spacing so it matches and doesn't result in the next PR having unrelated formatting changes would be good.

Once the formatting and example changes are pushed I'll be glad to merge!

@montanaflynn montanaflynn merged commit 0ffc3f2 into montanaflynn:master Jan 13, 2021
@montanaflynn
Copy link
Owner

I made the changes and added tests for arm64 on travisci, and they all passed! 🎉

I've also merged the PR into master, they will be included in a new tagged release soon.

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

Successfully merging this pull request may close these issues.

A few tests fail in different architectures due to precision errors
5 participants