DiffRules.jl
DiffRules.jl copied to clipboard
Added some rules for scaled airy and bessel functions and incomplete gamma
Codecov Report
Merging #66 (813f4e8) into master (ea79b94) will increase coverage by
0.12%. The diff coverage is100.00%.
@@ Coverage Diff @@
## master #66 +/- ##
==========================================
+ Coverage 96.95% 97.07% +0.12%
==========================================
Files 3 3
Lines 164 171 +7
==========================================
+ Hits 159 166 +7
Misses 5 5
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/rules.jl | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update ea79b94...813f4e8. Read the comment docs.
Test failure on Julia 1.0:
Error During Test at /home/runner/work/DiffRules.jl/DiffRules.jl/test/runtests.jl:46
11
Test threw exception
12
Expression: isapprox(dy, finitediff((z->begin
13
SpecialFunctions.gamma(foo, z)
14
end), bar), rtol=0.05)
15
MethodError: no method matching gamma(::Int64, ::Float64)
16
Closest candidates are:
17
gamma(::Union{Int16, Int32, Int64, Int8, UInt16, UInt32, UInt64, UInt8}) at /home/runner/.julia/packages/SpecialFunctions/ne2iw/src/gamma.jl:857
18
gamma(::Real) at /home/runner/.julia/packages/SpecialFunctions/ne2iw/src/gamma.jl:561
19
Stacktrace:
20
[1] (::getfield(Main, Symbol("##60#62")))(::Float64) at ./none:0
21
[2] finitediff(::getfield(Main, Symbol("##60#62")), ::Float64) at /home/runner/work/DiffRules.jl/DiffRules.jl/test/runtests.jl:15
22
Were these methods added to SpecialFunctions in a version which doesn't support Julia 1.0?
Should I qualify some rules with something like
if VERSION < v"0.7-"
as you have for atan2?
atan2 was in Base a long long time ago, and doesn't exist anymore, really that should be deleted entirely.
For gamma, when were these methods added to SpecialFunctions?
I don't think you can just branch on the version of Julia, unless somehow that version of SpecialFunctions doesn't support newer Julia versions. But you could make the tests check what methods exist before testing.
If you guide me to know when the method was added, I can do that. Same for the tests. :)
Maybe it's enough to test hasmethod(gamma, Tuple{Number, Number}) in the tests.
It doesn't quite belong in non_numeric_arg_functions , and unlike rem2pi you don't really want a different test, you just want to skip the normal test if this doesn't exist. Perhaps start another list for this, and any other such, something like:
recently_defined_functions = []
if !hasmethod(SpecialFunctions.gamma, Tuple{Number, Number})
# 2-arg gamma added in ??,
push!(recently_defined_functions, (...))
end
Re versions my thinking is that if in future this stops supporting some old versions of SpecialFunctions, then it's nice to know what bits you can delete. I looked through the closed PRs but this didn't jump out. I think the way to find out is git blame, which when viewing the source on github's web page is a button top-right.
I installed Julia 1.0 to test locally and SpecialFunctions was downgraded to v0.8.0. In that version gamma only has methods for one argument.
Now it's other packages don't pass the tests :(
same problem with the 2-arg gamma? Doesn't seem like it.
Same test in Symbolics.jl failing on CI for an unrelated PR: https://github.com/JuliaSymbolics/Symbolics.jl/pull/373/checks?check_run_id=3563295246
Ditto for ModelingToolkit.jl failure: https://github.com/SciML/ModelingToolkit.jl/pull/1252/checks?check_run_id=3528813219
So what can I do now?
I resolved the conflicts and removed the check for gamma(a, x): We should only define a rule for it if it is defined, so IMO the rule should not exist if SpecialFunctions.gamma(a, x) does not exist. As discussed in e.g. the PR for logerfcx the cleanest approach is to update the compat entry for SpecialFunctions. I updated it to 1.2, 2 since gamma(a, x) was added in SpecialFunctions 1.2.0: https://github.com/JuliaMath/SpecialFunctions.jl/compare/v1.1.0...v1.2.0
The ReverseDiff test errors are caused by (correct) NaN values. This is not a problem of this PR but rather of ReverseDiff's tests: https://github.com/JuliaDiff/ReverseDiff.jl/blob/d522508aa6fea16e9716607cdd27d63453bb61e6/test/utils.jl#L9-L13 Maybe this could be fixed (and also the currently skipped functions tested properly) if one uses isapprox(A, B; nans=true, atol=_atol) in https://github.com/JuliaDiff/ReverseDiff.jl/blob/d522508aa6fea16e9716607cdd27d63453bb61e6/test/utils.jl#L21?
@mcabbott Are you fine with updating the SpecialFunctions compat entry to "1.2, 2"?