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

Numerical uncertainty causing Travis CI to fail #6

Closed
rprechelt opened this issue Mar 3, 2016 · 3 comments
Closed

Numerical uncertainty causing Travis CI to fail #6

rprechelt opened this issue Mar 3, 2016 · 3 comments

Comments

@rprechelt
Copy link
Collaborator

When cloning the repo onto a new machine, I noticed the failing Travis CI badge and decided to take a look. As you may know, the failure is a test not generating the correct expected output; in this case, the sin() function is causing Travis CI to fail. I diff'ed the expected and computed arrays and the differences are of the order of 1e-16 and are "randomly" distributed; this would suggest that this build error is just floating-point inaccuracies.

Considering that roughly() calls base.isapprox(), are you aware of a method that we could implement to both fix this test error, and to prevent similar test errors from arising? I'd be happy to make a PR with a solution, but beyond writing a custom floating-point test suite with slightly larger acceptable error, I don't know a method of fixing this.

@simonbyrne
Copy link
Contributor

Thanks for the interest!

So, the problem is a little more complicated than floating point error: the line that's failing on Travis is actually this one, the purpose of which was to check that we're actually calling the correct method after overwriting the Base.sin method. Unfortunately, it seems like this isn't actually working correctly.

More generally, these tests should probably be rewritten to use BaseTextNext.jl, as FactCheck is slowly being deprecated.

@rprechelt
Copy link
Collaborator Author

Ah; that makes sense! This is what happens when I just glance over this stuff late at night! My apologies for the "false" issue notice.

That being said, I'd be happy to tackle rewriting these tests using BaseTextNext.jl; I have been using AppleAccelerate.jl a lot for the last few weeks and would love to give back.

I have already implemented a number of other Accelerate functions (conv, xcorr, acorr and others) in my local repo; if these are functions you think we should merge in, I'll write some tests for them and make a PR.

@simonbyrne
Copy link
Contributor

That would be great, thanks, contributions are certainly welcome. I think it would make sense to expose as much of Accelerate as possible.

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

No branches or pull requests

2 participants