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

Qualify special functions with module name. #18

Merged
merged 1 commit into from
Jun 29, 2018
Merged

Qualify special functions with module name. #18

merged 1 commit into from
Jun 29, 2018

Conversation

tpapp
Copy link
Contributor

@tpapp tpapp commented Jun 28, 2018

Fixes #17.

Lines were broken because they became too long.

Fixes #17.

Lines were broken because they became too long.
@tpapp
Copy link
Contributor Author

tpapp commented Jun 28, 2018

Someone knowledgeable please review the line

@define_diffrule NaNMath.lgamma(x) = :(  SpecialFunctions.digamma($x)               )

because I am not sure about the corner cases.

@jrevels
Copy link
Member

jrevels commented Jun 28, 2018

I assume that that's fine, but I'll ask @mlubin since his packages - e.g. JuMP - are the main users of these definitions.

@mlubin
Copy link

mlubin commented Jun 28, 2018

Actually JuMP doesn't currently use DiffRules, although we'd like to. But anyway is the issue that digamma is missing from NaNMath?

@tpapp
Copy link
Contributor Author

tpapp commented Jun 29, 2018

The issue is that

ForwardDiff.derivative(NaNMath.lgamma, x)

may be nonsensical for x where NaNMath.lgamma differs from SpecialFunctions.lgamma, since it uses SpecialFunctioncs.digamma. But it may just work if the analytic extension matches. I don't know enough about the gamma function to answer this.

In any case, unless there are objections, it would be great to merge this PR. It fixes some bugs, and I don't think it makes anything worse.

@mlubin
Copy link

mlubin commented Jun 29, 2018

I don't know enough about lgamma or digamma either, so no objection here.

@andreasnoack
Copy link
Member

Would be great to get this one in since it is holding back downstream packages.

@jrevels
Copy link
Member

jrevels commented Jun 29, 2018

Actually JuMP doesn't currently use DiffRules, although we'd like to.

JuMP uses ForwardDiff, which uses DiffRules. But anyway, this seems fine. Thanks again @tpapp!

@jrevels jrevels merged commit f8d7808 into JuliaDiff:master Jun 29, 2018
@tpapp tpapp deleted the tp/fix-rule-module branch July 6, 2018 08:27
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

4 participants