Allow arguments of type Float16 and Float32 in R functions
#124
Conversation
src/rmath.jl
Outdated
| @@ -6,6 +6,8 @@ import Rmath: libRmath | |||
|
|
|||
| ### import macro | |||
|
|
|||
| const FloatOrInt = Union{Float16,Float32,Float64,Int} | |||
There was a problem hiding this comment.
Any reason not to accept smaller integer types? Maybe we could even allow any Real, and let the conversion throw in case of overflow?
There was a problem hiding this comment.
I did not know where to stop so I just started with the types that would fix the linked issue in Distributions 😄 I was wondering about smaller integer types as well. I think Real would break stuff and lead to method ambiguities since for some of these functions we already define Julia implementations that accept Real arguments in StatsFuns (they are used e.g. for Dual numbers).
There was a problem hiding this comment.
Hmm, then why not use the Julia implementations when they exist? Is it faster to call Rmath even if that involves conversions?
There was a problem hiding this comment.
This is the motivation for #113 which I did not want to touch or duplicate in this PR. I haven't done any benchmarks but in my opinion performance should not be the main criterion for switching to Julia implementations. For instance, we should make sure that the Julia implementation is at least as accurate as the Rmath version, ideally on the whole input domain.
There was a problem hiding this comment.
Yeah #113 is complex enough that it's better to leave it alone. I'm just wondering what's the criterion to add some types to the list but not others. Maybe at least add small integer types?
There was a problem hiding this comment.
I added IntXs, UIntXs, and Rationals to the union as well since it would fix e.g. issues such as JuliaStats/Distributions.jl#1387 (comment). I am not particularly happy about the hardcoded union and hence used Real instead whenever no Julia fallback existed.
There was a problem hiding this comment.
OK, thanks. Some Rational{Int} values cannot be converted to Float64 without a loss in precision, but maybe we don't care, just like promote_type(Int64, Float64) == Float64 despite the precision loss for large values?
It would be nice to test the new types. :-)
…tional`s otherwise
Codecov Report
@@ Coverage Diff @@
## master #124 +/- ##
==========================================
+ Coverage 30.23% 34.25% +4.01%
==========================================
Files 12 12
Lines 377 397 +20
==========================================
+ Hits 114 136 +22
+ Misses 263 261 -2
Continue to review full report at Codecov.
|
| @@ -109,6 +140,9 @@ macro import_rmath(rname, jname, pargs...) | |||
| esc(_import_rmath(rname, jname, pargs)) | |||
| end | |||
|
|
|||
| macro import_rmath_real(rname, jname, pargs...) | |||
There was a problem hiding this comment.
| macro import_rmath_real(rname, jname, pargs...) | |
| # Used when no Julia fallback exists currently | |
| macro import_rmath_real(rname, jname, pargs...) |
src/rmath.jl
Outdated
| @@ -6,6 +6,8 @@ import Rmath: libRmath | |||
|
|
|||
| ### import macro | |||
|
|
|||
| const FloatOrInt = Union{Float16,Float32,Float64,Int} | |||
There was a problem hiding this comment.
OK, thanks. Some Rational{Int} values cannot be converted to Float64 without a loss in precision, but maybe we don't care, just like promote_type(Int64, Float64) == Float64 despite the precision loss for large values?
It would be nice to test the new types. :-)
|
Replaced and fixed by #125. |
Currently it is not possible to call R functions such as
nchisqlogpdfwith arguments of typeFloat16orFloat32which makes it impossible to use these types in e.g.NoncentralChisqin Distributions (JuliaStats/Distributions.jl#1387 (comment)). This PR fixes the problem.