-
Notifications
You must be signed in to change notification settings - Fork 35
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
Option to use system libm. #125
Conversation
This looks really nice, thank you for taking the time to submit a patch! Just to be certain, do the tests ( |
I noticed that a test for math fails, namely It's related to rounding, but I'm not sure I understand what exactly the issue is. Here is the output:
|
Can you post On Sat, Nov 8, 2014 at 12:59 AM, Nicolas Tessore notifications@github.com
|
I saw that same failure last time I tried |
|
Even the |
I'm merging this as-is for now, if we decide the precision is good enough, we can alter our tests and use system libm as default. In any case, thank you for your contribution! |
Checking what actually gets called in both cases:
Checking combinatorics.jl:45 function gamma(n::Union(Int8,UInt8,Int16,UInt16,Int32,UInt32,Int64,UInt64))
n < 0 && throw(DomainError())
n == 0 && return Inf
n <= 2 && return 1.0
n > 20 && return gamma(float64(n))
@inbounds return float64(_fact_table64[n-1])
end And there we see that the factorial table is in integers, which round more nicely than the returned Float64s from libm. |
Cc: @alanedelman @stevengj @jiahao @andreasnoack We should file this as an issue in base julia, if required. |
Thanks to the helpful pointer in JuliaLang/julia#8869, I added an option for
USE_SYSTEM_LIBM=1
. Since this makes numerical programs quite a bit faster on Macs, I would even suggest to make this the default behaviour, and add another--with-openlibm
flag if necessary.