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 differentiation rules from ChainRules #238

Merged
merged 6 commits into from
Dec 4, 2020

Conversation

devmotion
Copy link
Member

Currently, ChainRules defines a set of differentiation rules for functions from SpecialFunctions. As noted in the README and mentioned in this discussion, ideally these rules would be defined in SpecialFunctions. This PR adds these rules that are currently defined in ChainRules together with their corresponding tests to SpecialFunctions, with the following modifications:

  • the rule for lgamma is removed and the one for lbeta replaced with one for logbeta since lgamma and lbeta are deprecated
  • tests for beta and logbeta are added

I'm not sure if the maintainers of SpecialFunctions are willing to take a dependency on ChainRulesCore (it is designed as a lightweight interface for AD with LinearAlgebra and MuladdMacro as its only dependencies) and include these differentiation rules in their code base. This PR is motivated by an issue about potentially defining derivatives of Bessel functions with respect to the order as well that I opened a while ago in the ChainRules repo. In this issue, it was suggested to first move the code base to SpecialFunctions before modifying or extending the existing set of rules.

@devmotion
Copy link
Member Author

BTW I'm not sure what's the best way to avoid rule redefinition for users with old versions of ChainRules since it does neither depend on nor bound SpecialFunctions.

@oxinabox
Copy link
Contributor

BTW I'm not sure what's the best way to avoid rule redefinition for users with old versions of ChainRules since it does neither depend on nor bound SpecialFunctions.

On the ChainRules side we can check for: isdefined(SpecialFunctions, :ChainRulesCore) and if that is defined not define the rules in ChainRules.jl. (and later we can drop thise rules as a breaking change)
This still leaves the case of someone with a version of ChainRules that is too old to have that isdefined check,
but those users will just get warnings and should be told to update

@ararslan
Copy link
Member

I'm not sure if the maintainers of SpecialFunctions are willing to take a dependency on ChainRulesCore (it is designed as a lightweight interface for AD with LinearAlgebra and MuladdMacro as its only dependencies) and include these differentiation rules in their code base.

-1 from me. Integrating things like this should be in a separate package, not in a core package this far down the dependency tree of so many other packages.

@giordano
Copy link
Member

Not a maintainer here, but it looks a bit weird also to me if such functionality was here, since this isn't needed for SpecialFunctions.jl to work. However, we need to find a solution to this ecosystem problem, as it may come up with other packages as well.

@oxinabox
Copy link
Contributor

Its no different to TimeZones.jl having a dependency on RecipieBase.
Its a super lightweight dependency, to furfill making the ecosystme work out well.

Which I know @ararslan would also disagree with, and I think it is a legit enough disagreement.

@stevengj
Copy link
Member

stevengj commented Nov 2, 2020

A lightweight dependency on ChainRulesCore seems fine to me — that's what it's for, and I expect that more and more packages will be adding this dependency in the future.

@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #238 (c0d3b81) into master (eff906e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #238   +/-   ##
=======================================
  Coverage   87.68%   87.68%           
=======================================
  Files          11       11           
  Lines        2526     2526           
=======================================
  Hits         2215     2215           
  Misses        311      311           
Flag Coverage Δ
unittests 87.68% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/SpecialFunctions.jl 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 eff906e...c0d3b81. Read the comment docs.

@stevengj
Copy link
Member

stevengj commented Nov 3, 2020

To elaborate — going forward, I expect that more and more there will be an expectation that basic numerical functions will be differentiable, and the easiest place to do that is in the packages themselves. Putting the derivatives in a separate package just makes it harder to keep in sync, while eliminating a dependency on ChainRulesCore that most users will have anyway.

src/chainrules.jl Outdated Show resolved Hide resolved
@stevengj
Copy link
Member

Looks good to merge?

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.

5 participants