search

JuliaLinearAlgebra / AppleAccelerate.jl

Julia interface to the macOS Accelerate framework
Other
96 stars 18 forks source link

Numerical uncertainty causing Travis CI to fail #6

Closed rprechelt closed 8 years ago

rprechelt commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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