JuliaDiff / DiffRules.jl

A simple shared suite of common derivative definitions
Other
74 stars 38 forks source link

Add missing rules for SpecialFunctions 0.10 #79

Closed devmotion closed 1 year ago

devmotion commented 2 years ago

This PR is both an alternative and an extension of https://github.com/JuliaDiff/DiffRules.jl/pull/66.

It adds missing rules for functions defined in SpecialFunctions >= 0.10: https://specialfunctions.juliamath.org/v0.10/functions_list

Hence in contrast to #66 it does not add a rule for the incomplete gamma function which is only available in SpecialFunctions >= 1.2 and also does not require to update the SpecialFunctions compat entry.

I went through the list of functions in SpecialFunctions 0.10 and added rules that were missing for functions that are differentiable with respect to at least one argument and return scalar values (some functions return tuples but AFAIK these are not supported by DiffRules). Some functions are not holomorphic (e.g. besselix) and hence derivatives are only correct for real-valued inputs. It seems DiffRules does not support non-holomorphic functions and hence I only copied the rules for real inputs but not the ones for complex inputs from e.g. https://github.com/JuliaMath/SpecialFunctions.jl/blob/1febdd05902f5b3a3adc9915aba40b776aa3ea24/src/chainrules.jl#L198-L223; I also added some explanations to the code.

To make sure that the rules are defined correctly I added FiniteDifferences as a test dependency and decreased relative and absolute tolerances to 1e-9 (not possible with the current basic finite differencing algorithm).

codecov-commenter commented 2 years ago

Codecov Report

Base: 97.00% // Head: 97.31% // Increases project coverage by +0.30% :tada:

Coverage data is based on head (ea759af) compared to base (88ad24c). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #79 +/- ## ========================================== + Coverage 97.00% 97.31% +0.30% ========================================== Files 3 3 Lines 167 186 +19 ========================================== + Hits 162 181 +19 Misses 5 5 ``` | [Impacted Files](https://codecov.io/gh/JuliaDiff/DiffRules.jl/pull/79?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/rules.jl](https://codecov.io/gh/JuliaDiff/DiffRules.jl/pull/79/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL3J1bGVzLmps) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

devmotion commented 2 years ago

ForwardDiff tests should be fixed by https://github.com/JuliaDiff/ForwardDiff.jl/pull/568.

devmotion commented 2 years ago

Bump :slightly_smiling_face:

Recently multiple users reported problems on Slack with differentiating some of the special functions added in this PR, e.g., logerfc or besselix.

ReverseDiff and ForwardDiff define and test all rules unconditionally - even for functions with complex outputs. Therefore tests fail.

devmotion commented 2 years ago

I'll prepare PRs to ForwardDiff and ReverseDiff that fix the test errors before merging this PR.

devmotion commented 2 years ago

I made a PR that fixes ForwardDiff more generally: https://github.com/JuliaDiff/ForwardDiff.jl/pull/577

andreasnoack commented 2 years ago

Let's try to rerun CI here

devmotion commented 2 years ago

ForwardDiff test errors would be fixed by https://github.com/JuliaDiff/ForwardDiff.jl/pull/577 (or adding more things to the manual blacklist in the ForwardDiff tests, as done currently with other functions with complex-valued outputs).

Similarly, ReverseDiff tests would pass if either support for intermediate complex results for DiffRules-rules is added to ReverseDiff or these functions are excluded from the tests (as done currently).

andreasnoack commented 2 years ago

What should we do with the ReverseDiff tests? Will you prepare a similar PR?

devmotion commented 2 years ago

I'm not sure if ReverseDiff can be updated similarly without major changes. IIRC the type signatures were too restrictive when I checked it a while ago. Maybe the easiest way forward would be to just disable the failing rules for now in ReverseDiff.

devmotion commented 1 year ago

I had a look at the ReverseDiff test errors, it seems https://github.com/JuliaDiff/ReverseDiff.jl/pull/209 would fix all of the issues related to this PR here.

devmotion commented 1 year ago

With the latest release of ReverseDiff tests pass. The test failures of EAGO seem unrelated.

devmotion commented 1 year ago

Since you commented above @andreasnoack: Do you think the PR can be merged now?