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

Add missing rules for SpecialFunctions 0.10 #79

Merged
merged 20 commits into from
Oct 18, 2022
Merged

Conversation

devmotion
Copy link
Member

This PR is both an alternative and an extension of #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
Copy link

codecov-commenter commented Dec 12, 2021

Codecov Report

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

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
@@            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 Coverage Δ
src/rules.jl 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

@devmotion
Copy link
Member Author

ForwardDiff tests should be fixed by JuliaDiff/ForwardDiff.jl#568.

torfjelde added a commit to torfjelde/DiffRules.jl that referenced this pull request Jan 15, 2022
devmotion added a commit that referenced this pull request Jan 28, 2022
* fixed issues with return-type for several rules

* initial work on inferring return type using intermediate computations

* removed oftype where possible

* fixed stupid mistake

* missed one in fix of stupid mistake

* more fixes

* added tests for different types

* removed a couple of overdone oftypes

* moved a single paranthesis

* use irrationals to simplify type-promotion

* depend on IrrationalConstants.jl

* fixed tests

* actually fixed tests

* updated rules to use constants from IrrationalConstants

* Update src/rules.jl

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* reverted some changes from previous commit

* fixed a typo

* fixed typo

* fixed type-conversion in tests

* Apply suggestions from code review

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* reverse previous commit due to JuliaMath/NaNMath.jl#47

* reverse previous commit due to JuliaMath/NaNMath.jl#47

* drop qualifications from rules

* reverted unintended change to _abs_deriv

* interpolate _abs_deriv

* be explicit about imported irrationals

* fixed tests

* fixed numerical issues in tests by adopting some changes from #79

* relax rtol slightly since we're working with Float32 too here

* Update Project.toml

* test type of derivative for functions with 2 arguments

* fixed types of derivatives for mod, rem and different bessel functions

* use deg2rad

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* reverted changes to + and -

* remove duplicate rules

* add back whitespace

* reverted changes to bessel functions

* only test return-type having the correct promotion behavior

* only test type for 2 argument functions whose derivatives aren't NaN

* fixed rules of mod and rem

* make each rule its own testset for easier debugging

* reverted changes to multiple NaNMath rules

* use more explicit promotion in tests

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* check promotion of real instead of specific check for Complex

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>

* reverted unnecessary change

* reverted unnecessary changes

* dont check if AbstractFloat in tests

Co-authored-by: David Widmann <devmotion@users.noreply.github.com>
@devmotion
Copy link
Member Author

Bump 🙂

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.

Copy link
Member

@mcabbott mcabbott left a comment

Choose a reason for hiding this comment

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

This looks fine to me. I haven't checked the maths, but happy to believe you have, and that tests will catch most things.

I think ForwardDiff only defines rules for f(::Dual) and only allows Dual(::Real), hence Complex{<:Dual} not the reverse.

(Seems fine to increase bound on SpecialFuncitons too, but that can be later.)

@devmotion
Copy link
Member Author

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

@devmotion
Copy link
Member Author

I made a PR that fixes ForwardDiff more generally: JuliaDiff/ForwardDiff.jl#577

@andreasnoack
Copy link
Member

Let's try to rerun CI here

@devmotion
Copy link
Member Author

ForwardDiff test errors would be fixed by JuliaDiff/ForwardDiff.jl#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
Copy link
Member

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

@devmotion
Copy link
Member Author

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
Copy link
Member Author

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

@devmotion
Copy link
Member Author

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

@devmotion
Copy link
Member Author

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

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.

None yet

5 participants