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

clarify @define_diffrule namespaces #17

Closed
tpapp opened this issue Jun 28, 2018 · 4 comments
Closed

clarify @define_diffrule namespaces #17

tpapp opened this issue Jun 28, 2018 · 4 comments

Comments

@tpapp
Copy link
Contributor

tpapp commented Jun 28, 2018

I think that eg

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

should be

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

since the RHS is not necessarily evaluated in the module of the LHS. Is this correct?

Just please clarify, I would be happy to make a PR.

(cf JuliaStats/StatsFuns.jl#49)

@jrevels
Copy link
Member

jrevels commented Jun 28, 2018

Yup, good catch! The other rules should have this as well.

@tpapp
Copy link
Contributor Author

tpapp commented Jun 28, 2018

Can you help me figure out why this wasn't caught by the unit tests? I think they should be updated first, so that we get an error, which is then fixed.

@jrevels
Copy link
Member

jrevels commented Jun 28, 2018

I think it's because the tests do using SpecialFunctions, which brings all of these bindings into the testing namespace. IIUC, we should be able to fix that just by changing it to import SpecialFunctions.

@tpapp
Copy link
Contributor Author

tpapp commented Jun 28, 2018

Indeed, import breaks the tests, I made a PR which fixes things.

A review would be appreciated, especially for the case I mention in the PR discussion.

jrevels pushed a commit that referenced this issue Jun 29, 2018
Fixes #17.

Lines were broken because they became too long.
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

No branches or pull requests

2 participants