JuliaDiff / DiffRules.jl

A simple shared suite of common derivative definitions
Other
74 stars 38 forks source link

Preserve input types for various rules #89

Closed ptiede closed 1 year ago

ptiede commented 1 year ago

This pull request ensures that the input type is preserved for various rules. Previously there were potentially a few places where 64 bit NaN's would always be produced regardless of the input. To fix this I replaced any instance of :NaN with oftype($x, NaN).

Additionally, this pull-request fixes the issue with the ldexp rule from #88 which is similar in nature.

codecov-commenter commented 1 year ago

Codecov Report

Base: 97.31% // Head: 97.31% // No change to project coverage :thumbsup:

Coverage data is based on head (5767bca) compared to base (489e294). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #89 +/- ## ======================================= Coverage 97.31% 97.31% ======================================= Files 3 3 Lines 186 186 ======================================= Hits 181 181 Misses 5 5 ``` | [Impacted Files](https://codecov.io/gh/JuliaDiff/DiffRules.jl/pull/89?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/rules.jl](https://codecov.io/gh/JuliaDiff/DiffRules.jl/pull/89/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL3J1bGVzLmps) | `100.00% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

devmotion commented 1 year ago

To fix this I replaced any instance of :NaN with oftype($x, NaN).

:NaN is the DiffRules way to say that some derivative does not exist or is not implemented. You never want to use these "derivatives" anyway, every time you do you are already screwed.

Generally, oftype($x, NaN) seems like the wrong approach as x might not even be a floating point number. Just using float($x) instead seems also wrong in the two-argument functions since it might cause undesired promotions if x is not a floating point number (imagine eg the other argument being of type Float32).

ptiede commented 1 year ago

Ok I reverted the :NaN for the two argument functions.

However for the ldexp I think the point remains. In that case you do need the oftype(float($x), exp2($y)) to prevent a spurious promotion for the derivative with respect to x.