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

Numerical precision of primal not respected for SpecialFunctions.jl #402

Closed
torfjelde opened this issue Apr 25, 2021 · 4 comments · Fixed by #425
Closed

Numerical precision of primal not respected for SpecialFunctions.jl #402

torfjelde opened this issue Apr 25, 2021 · 4 comments · Fixed by #425

Comments

@torfjelde
Copy link

Several methods from SpecialFunctions.jl are not respecting the numerical precision of the primal:

julia> using ChainRules, SpecialFunctions

julia> y, ȳ = ChainRules.rrule(SpecialFunctions.erf, 1.0f0)
(0.8427008f0, SpecialFunctions.var"#erf_pullback#12"{Float32}(1.0f0))

julia> (1.0f0)[2]
0.41510750774498784

I'm guessing this is because the rules were originally copy-pasted from DiffRules.jl which suffers from the same issue: JuliaDiff/DiffRules.jl#55.

I think I'm currently of the opinion that the best way to fix this is to copy-paste the constants from e.g. StatsFuns.jl to remove most of the promotions, and then I guess we'll have to use oftype for the remainders.

Is there a better solution?

Btw, I know this related to other issues, e.g. #232, but to me it seems like this particular case is non-ambiguous and can be easily fixed so might as well do it asap:)

@devmotion
Copy link
Member

The differentials were moved to SpecialFunctions (JuliaMath/SpecialFunctions.jl#238) and are only loaded from ChainRules if an old version of SpecialFunctions without them is used. So all problems have to be fixed there as well.

@devmotion
Copy link
Member

I think I'm currently of the opinion that the best way to fix this is to copy-paste the constants from e.g. StatsFuns.jl to remove most of the promotions

Instead of copy-pasting the definitions one could just use oftype (as you suggested) or move the constants to a separate package (MathConstants.jl? StatsConstants.jl?). BTW log-related constants live in LogExpFunctions now and are only reexported by StatsFuns.

@torfjelde
Copy link
Author

The differentials were moved to SpecialFunctions

Ah, my bad! Should I just close this and open one over there then? Though I guess the rules also needs to be changed here.

Instead of copy-pasting the definitions one could just use oftype (as you suggested) or move the constants to a separate package (MathConstants.jl? StatsConstants.jl?).

I was sort of surprised to find that these were in fact not all in one package, so I would be cool with that. Would be suuuper-lightweight too. Initially I thought "eh, it's dependent on the stats packages actually using this", but I guess we could just start using it AD-frameworks immediately.

BTW log-related constants live in LogExpFunctions now and are only reexported by StatsFuns.

Cool 👍 But StatsFuns also has its own constants, etc. so it's a bit spread around.

@mzgubic
Copy link
Member

mzgubic commented Jun 1, 2021

We are removing the SpecialFunctions rules in this package, have you transferred the issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants