JuliaDiff / ReverseDiff.jl

Reverse Mode Automatic Differentiation for Julia
Other
348 stars 57 forks source link

Improve DiffRules integration and tests #209

Closed devmotion closed 2 years ago

devmotion commented 2 years ago

This PR fixes some problems with the DiffRules integration and its tests. It is needed for https://github.com/JuliaDiff/DiffRules.jl/pull/79 (relevant DiffRules tests pass with that PR).

Mainly, the PR

The vcat test error is unrelated and also present on the master branch and other PRs. Edit: Fixed on the master branch,

I also assume we could do better than ForwardDiff here and also avoid that all results become NaN if derivatives are computed with respect to both arguments and only one is defined/exists. But replacing ForwardDiff with a direct implementation of the DiffRules-derivatives seemed to require much larger changes, and I tried to apply only a somewhat minimal set of changes required for https://github.com/JuliaDiff/DiffRules.jl/pull/79.

codecov-commenter commented 2 years ago

Codecov Report

Base: 85.16% // Head: 81.24% // Decreases project coverage by -3.92% :warning:

Coverage data is based on head (088182c) compared to base (8ac1f7d). Patch coverage: 58.06% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #209 +/- ## ========================================== - Coverage 85.16% 81.24% -3.93% ========================================== Files 18 18 Lines 1861 1578 -283 ========================================== - Hits 1585 1282 -303 - Misses 276 296 +20 ``` | [Impacted Files](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | Coverage Δ | | |---|---|---| | [src/ReverseDiff.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL1JldmVyc2VEaWZmLmps) | `100.00% <ø> (ø)` | | | [src/derivatives/scalars.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2Rlcml2YXRpdmVzL3NjYWxhcnMuamw=) | `95.74% <ø> (-0.98%)` | :arrow_down: | | [src/derivatives/elementwise.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2Rlcml2YXRpdmVzL2VsZW1lbnR3aXNlLmps) | `74.71% <58.06%> (-4.16%)` | :arrow_down: | | [src/derivatives/broadcast.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2Rlcml2YXRpdmVzL2Jyb2FkY2FzdC5qbA==) | `76.62% <0.00%> (-13.99%)` | :arrow_down: | | [src/tape.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL3RhcGUuamw=) | `55.55% <0.00%> (-9.16%)` | :arrow_down: | | [src/tracked.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL3RyYWNrZWQuamw=) | `86.81% <0.00%> (-5.49%)` | :arrow_down: | | [src/api/hessians.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2FwaS9oZXNzaWFucy5qbA==) | `84.00% <0.00%> (-3.50%)` | :arrow_down: | | [src/api/tape.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2FwaS90YXBlLmps) | `72.22% <0.00%> (-3.05%)` | :arrow_down: | | [src/derivatives/linalg/arithmetic.jl](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff#diff-c3JjL2Rlcml2YXRpdmVzL2xpbmFsZy9hcml0aG1ldGljLmps) | `69.67% <0.00%> (-1.45%)` | :arrow_down: | | ... and [10 more](https://codecov.io/gh/JuliaDiff/ReverseDiff.jl/pull/209/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaDiff) | | 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 2 years ago

Bump :slightly_smiling_face:

It would be good to fix ReverseDiff such that we can move forward with https://github.com/JuliaDiff/DiffRules.jl/pull/79.

mohamed82008 commented 2 years ago

Sorry for the delay, been swamped recently. I will take a look tonight.

mohamed82008 commented 2 years ago

Ah the methods are defined a bit further in the same file, nevermind.