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

Added some rules for scaled airy and bessel functions and incomplete gamma #66

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

amrods
Copy link

@amrods amrods commented Aug 27, 2021

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 27, 2021

Codecov Report

Merging #66 (813f4e8) into master (ea79b94) will increase coverage by 0.12%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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 data
Powered by Codecov. Last update ea79b94...813f4e8. Read the comment docs.

@mcabbott
Copy link
Member

mcabbott commented Sep 3, 2021

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?

@amrods
Copy link
Author

amrods commented Sep 4, 2021

Should I qualify some rules with something like

if VERSION < v"0.7-"

as you have for atan2?

@mcabbott
Copy link
Member

mcabbott commented Sep 4, 2021

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.

@amrods
Copy link
Author

amrods commented Sep 5, 2021

If you guide me to know when the method was added, I can do that. Same for the tests. :)

@mcabbott
Copy link
Member

mcabbott commented Sep 5, 2021

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.

@amrods
Copy link
Author

amrods commented Sep 6, 2021

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.

@amrods
Copy link
Author

amrods commented Sep 6, 2021

Now it's other packages don't pass the tests :(
same problem with the 2-arg gamma? Doesn't seem like it.

@mcabbott
Copy link
Member

@amrods
Copy link
Author

amrods commented Sep 12, 2021

So what can I do now?

src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
src/rules.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@devmotion
Copy link
Member

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: JuliaMath/SpecialFunctions.jl@v1.1.0...v1.2.0

@devmotion
Copy link
Member

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?

@devmotion
Copy link
Member

@mcabbott Are you fine with updating the SpecialFunctions compat entry to "1.2, 2"?

Comment on lines +177 to +178
@define_diffrule SpecialFunctions.hankelh1x(ν, x) =
:NaN, :( (SpecialFunctions.hankelh1x($ν - 1, $x) - SpecialFunctions.hankelh1x($ν + 1, $x)) / 2 - im * SpecialFunctions.hankelh1x($ν, $x) )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always assumed that DiffRules only deals with functions with real arguments and real outputs - but hankelh1x, hankelh2x, and also hankelh1 and hankelh2 for which a rule is already defined output complex numbers. So complex numbers are supported officially? It seems based on the derivatives for conj and adjoint that you added in #54 you also thought that DiffRules only deals with real numbers @mcabbott?

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

Successfully merging this pull request may close these issues.

4 participants