Closed david-hofmann closed 2 years ago
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!
Ah, no, nevermind, I see that some are still related to ldexp
. The ReverseDiff for instance.
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!
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 :shrug:). 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.
Then let's just merge and leave the rest to downstream?
Merging #73 (4861370) into master (ea79b94) will increase coverage by
0.01%
. The diff coverage is100.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.
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.
Thank you @david-hofmann!
Added a rule for the function ldexp