Fix ReverseDiff bug #278
Fix ReverseDiff bug #278
Conversation
devmotion
force-pushed
the
dw/fix_broadcast_try2
branch
from
24d4770
to
127bc0a
Compare
Codecov Report
@@ Coverage Diff @@
## master #278 +/- ##
==========================================
- Coverage 99.88% 95.52% -4.36%
==========================================
Files 5 5
Lines 857 849 -8
==========================================
- Hits 856 811 -45
- Misses 1 38 +37
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
The InfiniteLinearAlgebra downstream test failure seems unrelated? https://github.com/JuliaArrays/FillArrays.jl/actions/runs/5525483793/jobs/10079152899?pr=278 |
|
Yes, that should be fixed by JuliaArrays/LazyArrays.jl#257 |
|
What's your preference regarding tests? Should we add DistributionsAD (possibly only the ReverseDiff tests) to the downstream tests? Or would you be OK with adding the ReverseDiff examples in this PR and #273 to the FillArrays tests? |
|
@jishnub is there any issue with adding ReverseDiff as a test dependency? I checked and it doesn't depend on FillArrays.jl so I think its fine |
|
There's perhaps no fundamental problem with It would be ideal if we could test a small subsection that depends on |
|
So far I've only added the two MWE to the tests, neither the ReverseDiff nor the DistributionsAD-ReverseDiff (there exist tests for Zygote, Tracker, and ForwardDiff as well...) downstream tests are run. The main reason is that the former takes only a few seconds whereas both downstream tests would take around 45-60 minutes (and would also install more dependencies, I assume). Breaking down the DistributionsAD tests further, not only based on the AD backend but also the use of FillArrays, seems challenging since it is used a lot in the multivariate normal distributions but also in some other distributions, sometimes even only internally in Distributions. |
|
I think the present approach in this PR makes sense. Running the entire test suite of a large package might increase the maintenance burden of this package in case there are failures originating elsewhere. |
|
Could you also add some AD tests for +, along with the tests for - that are added in this PR? |
|
Done! |
Unfortunately, the last commit in #273 (which fixed type inference issues) broke a ReverseDiff example (slightly modified version of the MWE in #252). On the master branch:
Put simply, the general problem with broadcasting and AD is that most backends fall back to compute derivatives of the applied function with ForwardDiff - but the type of the output type is fixed to one that is incompatible with the internally used
Dualnumbers. This PR seems to fix the example above in an arguably simple way.In an ideal world, FillArrays would not have to care about these AD-specific details. AD packages should be improved and their broadcasting should be more stable. But I don't see this happening anytime soon given the number of contributors to packages such as ReverseDiff or Tracker, and the huge number of possibilities of breaking downstream code that is working fine right now. I'm not a maintainer of FillArrays but since all these FillArray-specific issues did not exist prior to FillArrays 1.0.1, it seems reasonable to me to apply these AD fixes directly in FillArrays.
Unfortunately, I don't see a good way of testing these ReverseDiff issues without adding AD tests or downstream tests on DistributionsAD.
I'll rerun the DistributionsAD tests with this PR in the DistributionsAD repo: TuringLang/DistributionsAD.jl#250