Fix for #95

[torfjelde]

This PR fixes #95 and removes the now redundant Zygote.jl-specific rules in favour of ChainRules.jl-only.

In the process I also improved the testing for AD a bit and noticed that ReverseDiff.jl will silently compute the wrong gradient when using getindex. I've currently just marked these cases as broken.

Also, IMO ChainRulesCore.jl should just be a direct dependency so compat-bounds can be added. #95 likely would have been avoided if that was the case, though I realize at the time of impl this might not have made as much sense.

EDIT: Also, is there a reason why Manifest.toml is used in test/? Maybe this should also be removed in favour of compat-bounds in test/Project.toml?

[torfjelde]

Produces a zero-vector rather than truth. Guessing there's something wrong with src/compat/reversediff.jl but I haven't investigated this further.

[codecov-commenter]

Codecov Report

Merging #96 (4d1bb3e) into master (37e3e4d) will increase coverage by 3.15%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master      #96      +/-   ##
==========================================
+ Coverage   70.95%   74.10%   +3.15%     
==========================================
  Files          20       19       -1     
  Lines         599      587      -12     
==========================================
+ Hits          425      435      +10     
+ Misses        174      152      -22     

Impacted Files	Coverage Δ
src/if_required/chainrulescore.jl	75.00% <50.00%> (+75.00%)	⬆️
src/array_interface.jl	84.67% <0.00%> (-0.81%)	⬇️
src/if_required/reversediff.jl	100.00% <0.00%> (+35.71%)	⬆️

---

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 37e3e4d...4d1bb3e. Read the comment docs.

[jonniedie]

Thanks for this!

I'm glad you caught that ReverseDiff issue. ReverseDiff support is pretty new for ComponentArrays, so it will be good to have better tests for that. I'll figure out what's going wrong with it.

Yes, I agree that ChainRulesCore should be a direct dependency. I've been meaning to move some of the low-dependency packages like this out of Requires and into direct dependencies.

No, there's no reason Manifest is being used in test/. That can change to using compat bounds.

Also, I'll try to track down why this failing for v1.2 and nightly. I guess it's not a huge deal if we need to drop support for v1.2, but ideally I'd like to keep it if at all possible.

[jonniedie]

Oh, nevermind on the v1.2 thing. That's only failing now because the Zygote tests (which were broken before on v1.2) are running again. I'll put a @test_broken for v1.2 so we can keep this.

[jonniedie]

Also, the nightly thing looks like it has to do with this, so I'm not worried about that either.

[torfjelde]

Good stuff!:)