Added ldexp to list of diffrules

[david-hofmann]

Added a rule for the function ldexp

[david-hofmann]

Dear @devmotion , I have now also added a special test rule for ldexp. I tested it locally and I think it works as it is supposed to but the test errors still remain. However, as far as I can see it doesn't have anything to do with my code additions or am I missing something? Can I do anything to move forward with the PR? Thank you!

[david-hofmann]

Ah, no, nevermind, I see that some are still related to ldexp. The ReverseDiff for instance.

[david-hofmann]

Okay, I see that at least the issues with the downstream tests of ForwardDiff and ReverseDiff are related to ldexp . It is unclear to me how those tests are run - on my local machine no such tests are run when executing runtests.jl . Locally I am passing all the tests (that are being run) successfully. Am I supposed to also alter the tests in ForwardDiff and ReverseDiff to make them compatible with ldexp? Thanks for any advice!

[devmotion]

Am I supposed to also alter the tests in ForwardDiff and ReverseDiff to make them compatible with ldexp?

The problem is that these packages define differentiation rules based on the rules in DiffRules automatically and then also test them automatically, without taking into account the types of the arguments of these functions. Of course, this is problematic already for existing rules such as the one for rem2pi and hence the rule for rem2pi is skipped in the ReverseDiff tests: https://github.com/JuliaDiff/ReverseDiff.jl/search?q=rem2pi (probably would be better to handle it separately but apparently this is not done currently 🤷). Similarly, in ForwardDiff the rule for rem2pi is not tested automatically: https://github.com/JuliaDiff/ForwardDiff.jl/search?q=rem2pi In ForwardDiff it is actually tested separately. Similarly, one would have to skip these automatic tests for ldexp and ideally test the rules for it separately.

It would be nice for maintainers of these downstream packages if you could prepare PRs that skip these automatic tests also for ldexp - but technically you don't have to: the addition of a new rule is considered non-breaking, and also in these cases it would not break these packages but only their automatic tests. When the rule for ldexp is available in DiffRules, then ideally one should add specific tests for ldexp to ReverseDiff and ForwardDiff to make sure that their automatic rules are correct.

[ChrisRackauckas]

Then let's just merge and leave the rest to downstream?

[codecov-commenter]

Codecov Report

Merging #73 (4861370) into master (ea79b94) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
+ Coverage   96.95%   96.96%   +0.01%     
==========================================
  Files           3        3              
  Lines         164      165       +1     
==========================================
+ Hits          159      160       +1     
  Misses          5        5              

Impacted Files	Coverage Δ
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 ea79b94...4861370. Read the comment docs.

[devmotion]

Then let's just merge and leave the rest to downstream?

Fine with me. I merged the master branch, this should fix the other test errors.

[mcabbott]

Seems OK, breaks ForwardDiff's tests but won't break actual usage.

[devmotion]

Thank you @david-hofmann!