torfjelde commented 3 years ago

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 commented 3 years ago

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 * π.

torfjelde commented 3 years ago

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

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?
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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

codecov-commenter commented 3 years ago

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 commented 2 years ago

What's the status of this PR? Can you resolve the conflicts and remove the constants since they are part of IrrationalConstants 0.1.1 @torfjelde?

torfjelde commented 2 years ago

I'll give it a go :+1:

torfjelde commented 2 years ago

I'm about to fly so I don't have time to wait for the results, but locally a couple of tests are failing and I'm a bit uncertain why.

SpecialFunctions.beta seems to fail. Unclear if it's just because the finitediff isn't accurate enough when using Float32.

torfjelde commented 2 years ago

@devmotion Doing stuff like $logten instead of $(DiffRules.logten) won't work unless you also have done using IrrationalConstants in the scope where you use eval. I recall now that this was the original reason why I added the module-specification.

devmotion commented 2 years ago

Doing stuff like $logten instead of $(DiffRules.logten) won't work unless you also have done using IrrationalConstants in the scope where you use eval. I recall now that this was the original reason why I added the module-specification.

Are you sure? I thought due to the $ the function is interpolated in the expression tree and will not refer to be resolved locally in the module.

If it doesn't work though then the alternative with $(DiffRules....) does not work generally either - DiffRules is not necessarily available in the module where you eval the rules. For instance, it seems it would fail if you just load diffrules with using DiffRules: diffrules (and this example doesn't seem too far fetched).

torfjelde commented 2 years ago

Doing stuff like $logten instead of $(DiffRules.logten) won't work unless you also have done using IrrationalConstants in the scope where you use eval. I recall now that this was the original reason why I added the module-specification.

This won't work though - DiffRules is not necessarily available in the module where you eval the rules. For instance, it seems it would fail if you just load diffrules with using DiffRules: diffrules (and this example doesn't seem too far fetched).

Sure, but then we can't use this approach using irrationals?

devmotion commented 2 years ago

I still think it works just with standard interpolation, I'm not sure why it failed when you tried it previously (maybe you used e.g. :($x + logten) instead of :($x + $logten)?):

julia> module A
           f(x) = x
           g(x) = :($x + $f($x))
       end
Main.A

julia> module B
           using ..A: g
           @eval begin
               r(x) = $(g(:x))
           end
       end
Main.B

julia> B.r(1.0)
2.0

devmotion commented 2 years ago

Or with IrrationalConstants:

julia> module A
           using IrrationalConstants: logtwo
           g(x) = :($x + $logtwo)
       end
Main.A

julia> module B
           using ..A: g
           @eval begin
               r(x) = $(g(:x))
           end
       end
Main.B

julia> B.r(1.0)
1.6931471805599454

torfjelde commented 2 years ago

Okay I'm stupid. I'm still a bit confused as to why this works though. When I inspect the resulting Expr, i.e. before using eval, it looks like there's no reference to the original module, etc. So how does it then manage to resolve it back to where the irrational is defined?

Btw, though I agree that this is nicer, you still need to have DiffRules available in the scope when doing eval for general rules, e.g. https://github.com/JuliaDiff/DiffRules.jl/blob/7b1c31ed5b026ea445fd42c9674391a45dc9c536/src/rules.jl#L74.

devmotion commented 2 years ago

it looks like there's no reference to the original module, etc. So how does it then manage to resolve it back to where the irrational is defined?

It's just not displayed but it is actually a proper reference to the irrational.

Btw, though I agree that this is nicer, you still need to have DiffRules available in the scope when doing eval for general rules, e.g.

It seems if _abs_deriv is interpolated it's not necessary anymre that DiffRules is available.

torfjelde commented 2 years ago

It's just not displayed but it is actually a proper reference to the irrational.

Well, that's confusing :sweat_smile: But why isn't this the case when I do DiffRules.logten then? Shouldn't this also be the same?

It seems if _abs_deriv is interpolated it's not necessary anymre that DiffRules is available.

Oh of course!

devmotion commented 2 years ago

Shouldn't this also be the same?

Yes, but it's unnecessarily verbose. I was confused initially because it sounded like you used it since $logten did not work - but since both approaches result in the same expressions this did not make sense to me.

torfjelde commented 2 years ago

Shouldn't this also be the same?

Yes, but it's unnecessarily verbose. I was confused initially because it sounded like you used it since $logten did not work - but since both approaches result in the same expressions this did not make sense to me.

Yeah, it was because I only looked at the expanded expression and noted it had lost the reference. Sorry, not making that mistake again:) Thanks man!

torfjelde commented 2 years ago

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

devmotion commented 2 years ago

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

JuliaDiff / DiffRules.jl

Fixes return-types for multiple rules #55

Motivation

Aim

Up-for-discussion

Codecov Report