Accumulate derivative into Adjoint's original elements

[KDr2]

Try to fix #183.

[codecov-commenter]

Codecov Report

Merging #184 (de22270) into master (01041c8) will decrease coverage by 0.12%.
The diff coverage is 40.00%.

@@            Coverage Diff             @@
##           master     #184      +/-   ##
==========================================
- Coverage   84.36%   84.24%   -0.13%     
==========================================
  Files          18       18              
  Lines        1721     1726       +5     
==========================================
+ Hits         1452     1454       +2     
- Misses        269      272       +3     

Impacted Files	Coverage Δ
src/derivatives/propagation.jl	85.14% <25.00%> (-2.49%)	⬇️
src/api/tape.jl	75.00% <100.00%> (+0.28%)	⬆️

---

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 01041c8...de22270. Read the comment docs.

[KDr2]

The adjoint rule is causing other issues:
https://github.com/JuliaDiff/ReverseDiff.jl/pull/184/checks?check_run_id=3595074063#step:6:462

Another solution is that we don't deal with Adjoint as before, just leave it to ForwardDiff (also, as an optimization), to do so, we can change

for ... in DiffRules.diffrules()
    ...

to something like:

rules = ...DiffRules except [adjoint, conj]
for ... in rules
    ...

to fix the problem.

What do think @yebai @mohamed82008 @devmotion

[devmotion]

I don't think these rules should be excluded: #183 (comment)

[KDr2]

We have two ways to fix this:

1. accumulate the derivatives into Adjoint's original matrix
2. materialize Adjoint to a plain Array before doing broadcast

Because there's not a concise way to tackle all the broadcasting, I am trying the first one.

[devmotion]

This is a quite terrible type piracy 😥

[devmotion]

I'm not sure, since this is so unrelated to the other changes to me it seems generally a bit weird. Why do we have to "fix" lu if the problem is the accumulation of the derivatives? I understand (or assume) it is necessary to fix some test errors but I think this points to another deeper problem or at least requires a more general solution.

[KDr2]

Yeah, the introduction of adjoint in DiffRules also causes failure of another test case which is unrelative to broadcasting on Adjoint:
https://github.com/JuliaDiff/ReverseDiff.jl/blob/master/test/api/JacobianTests.jl#L293

My intention was indeed to fix it, but I am not familiar with LA and Jacobian Matrix, I just went through the error log and found it needs such a method. Sorry about that. I hope somebody would like to dig deeper into this. Thanks.

[devmotion]

No worries 🙂 I just found #183 (comment), so there's definitely a lot of not nice/non-general code regarding Adjoint already in ReverseDiff 🙈 Would make it even nicer to fix these problems more generally.

[devmotion]

#183 (also for transpose) will be fixed by JuliaDiff/DiffRules.jl#67.