Fixes return-types for multiple rules

[torfjelde]

Motivation

Currently, almost all of the rules using π currently has a return-type that differs from the input, e.g. SpecialFunctions.erfcx uses √π and so SpecialFunctions.erfcx(1f0) results in Float64 rather than Float32.

Aim

This PR fixes these issue by replacing all usages of π with oftype($x, π). This is a rather naive and quick-fix.

Up-for-discussion

• It might be preferable to instead exploit Julia's constant propagation, e.g. do √π is replaced by oftype($x, √π) instead of √oftype($x, π), but I don't know enough about LLVM to know if this makes a difference such simple computations, hence I just went with naive conversion of π directly.
• It would be ideal to have tests for this, but it's unclear to me if all unary rules are expected to have the same return-type as the input-type or not. It seems to me like this ought to be the case, but didn't add any yet as I'm not 100% certain.

[andreasnoack]

Thanks for fixing these. As a matter of style, I prefer to avoid explicit uses of typeof when possible. Sometimes it's the only way to get the type right but often you can simply adjust the order of evaluation in a way such that the promotion is correct. E.g. (π / 180) * cosd($x) could be changed to cosd($x) / 180 * π.

[andreasnoack]

I think this could fail for integer inputs (unless there is some promotion before the rules are hit). Generally, it's safer to use "output" variables for the promotion so it would be something like

y = 2 * $x * SpecialFunctions.erfcx($x))
return y - oftype(y, 2)/π

[torfjelde]

Sorry for the delay here; been a busy couple of months and so this was pushed quite far down my priority list.

But yeah, this is a good point. Is the best solution then to go through all of the rules and manually make these changes, or is there a better way?

[mcabbott]

Should we be concerned what happens if x is a substantial expression? This might evaluate it twice, which could be avoided by something like

@gensym z
@define_diffrule Base.sind(x)  = :(  $z=$x;  (oftype($z, π) / 180) * cosd($z)   )

I see that a few existing rules do already violate this, e.g. with exp(-$x * $x).

It's possible that @define_diffrule could be extended to take care of this. It's also possible that every downstream use calls CSE which will clean this up anyway.

[torfjelde]

I wasn't aware that x could potentially be an non-evaluated expression; if so, that's for sure an issue, but maybe a separate one from this PR?

[torfjelde]

Made some updates. There are a couple of things that:

1. Currently not testing binary methods properly because it's a bit unclear to how the types of the input arguments is decide in these tests. Seems like you want to sometimes have the first argument be a integer?
2. Added a @testset wrapper because I was a bit confused as to what threw which error. IMO the output is more understandable if something fails, but this can be removed if you want.

[torfjelde]

Actually, would it be better if I just added the constants needed, e.g. copy-paste those we need from https://github.com/JuliaStats/StatsFuns.jl/blob/master/src/constants.jl ?

That would solve almost all the promotion-bugs:)

[torfjelde]

An update here: am currently trying to convince people that it's a good idea to make a MathConstants.jl package so that we can avoid explicit conversions here, in SpecialFunctions.jl, etc.

[torfjelde]

I added the necessary irrationals to simplify the type-promotion. If you're happy to depend on IrrationalConstants.jl, we can make use of irrationals defined there instead:)

[torfjelde]

Now that packages have started using IrrationalConstants.jl, maybe this is ready for a proper consideration? @andreasnoack @mcabbott

[codecov-commenter]

Codecov Report

Merging #55 (5d7b635) into master (7b1c31e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #55   +/-   ##
=======================================
  Coverage   96.96%   96.96%           
=======================================
  Files           3        3           
  Lines         165      165           
=======================================
  Hits          160      160           
  Misses          5        5           

Impacted Files	Coverage Δ
src/DiffRules.jl	100.00% <ø> (ø)
src/rules.jl	100.00% <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 7b1c31e...5d7b635. Read the comment docs.

[devmotion]

Looks good, only some minor questions/suggestions left 🙂

[torfjelde]

Btw, seems like the integration tests for Symbolics.jl are broken.

[devmotion]

Btw, seems like the integration tests for Symbolics.jl are broken.

Yeah, seems it's a Symbolics issue, the same errors show up on their master branch: https://github.com/JuliaSymbolics/Symbolics.jl/runs/4975659103?check_suite_focus=true

[devmotion]

LGTM, thanks @torfjelde!