reactivate AD tests: mean functions

[st--]

Turns out there were a bunch of AD tests in the code. They were just commented out (and a bit broken). This is the work-in-progress attempt to reactivate them, and thereby resolve #311. Help welcome!

[codecov[bot]]

Codecov Report

Merging #313 (41a01da) into master (d99311e) will decrease coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #313      +/-   ##
==========================================
- Coverage   97.64%   97.62%   -0.02%     
==========================================
  Files          10       10              
  Lines         382      379       -3     
==========================================
- Hits          373      370       -3     
  Misses          9        9              

Impacted Files	Coverage Δ
src/mean_function.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 d99311e...41a01da. Read the comment docs.

[st--]

@devmotion you requested changes, but it's not clear to me what changes you're actually requesting, could you please clarify? regarding the definition of zero(::ColVecs), see my reply https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/pull/313#discussion_r844950873 though I also opened https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/pull/444

[devmotion]

I would like my comments to be addressed. Ie, no new commented out methods, no unrelated and unneeded changes, no new type piracy (doesn't matter if it's for tests or in the package), and understanding what causes the AD issues before reverting the FillArray changes (since possibly one can fix them in a better way).

[st--]

no new commented out methods,

I already said I was going to remove it before merging, just left it there while we were still discussing it so others could consider it.

no unrelated and unneeded changes,

I'm assuming you're referring to cleaning up the tests & making them more consistent (I was working towards being able to unify them further, instead of duplicating [test] code)? I've separated it out into https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/pull/315/files, and temporarily changed the PR target so that those changes don't show up in here.

no new type piracy (doesn't matter if it's for tests or in the package),

I already opened https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/pull/444 for this. I've now added tests there, and I'll of course update this PR once KernelFunctions has a new release.

and understanding what causes the AD issues before reverting the FillArray changes (since possibly one can fix them in a better way).

I've posted the stack traces, if you can make some sense of them, please enlighten me! But if it's not straightforward to fix (and at least to me it doesn't seem straightforward), I would think it makes more sense to add AD tests & get them to pass (as otherwise the package is currently broken), and then leave fixing the FillArray changes for later.

[st--]

@devmotion I believe that addressed all your concerns

[devmotion]

I've posted the stack traces, if you can make some sense of them, please enlighten me! But if it's not straightforward to fix (and at least to me it doesn't seem straightforward), I would think it makes more sense to add AD tests & get them to pass (as otherwise the package is currently broken), and then leave fixing the FillArray changes for later.

The stracktraces aren't too bad, are they? It's obviously a problem with the calls of FiniteDifferences in https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/blob/598cd63c71ea7dbde451403304df4d41ebe89909/test/test_util.jl#L54-L68. Just using ChainRulesTestUtils with its improved logic in https://github.com/JuliaDiff/ChainRulesTestUtils.jl/blob/main/src/finite_difference_calls.jl (also e.g. for NoTangents etc.) might be sufficient. rrules should be tested with ChainRulesTestUitls anyway instead of some custom Zygote-based tests.

[st--]

The stracktraces aren't too bad, are they?

I'm glad for you that you find parsing them so easy. If I had found it as easy to identify the issue & how to fix it, believe me, I would have just done that. I've already spent more time than I should have on trying to make the situation a bit better. If you want me to do it, all I can do at this point is propose this change to go back to fill() and zeros() and leave the "change to better ways of doing it & revert the FillArrays" to an open issue someone else can pick up at a later date.

[devmotion]

I unsubscribed, please find another reviewer.

[st--]

@willtebbutt thanks, looks like your collect() actually fixed the issues, and now it's all happy even with Zero and Fill. Also, I checked and neither of the two rrules defined in mean_function.jl for ZeroMean and ConstMean, respectively, are actually needed for the AD test to pass. Should we keep (/add) or remove them ?

Re https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/pull/313#discussion_r846658855 please let me know which way around you prefer.

[willtebbutt]

@willtebbutt thanks, looks like your collect() actually fixed the issues, and now it's all happy even with Zero and Fill. Also, I checked and neither of the two rrules defined in mean_function.jl for ZeroMean and ConstMean, respectively, are actually needed for the AD test to pass. Should we keep (/add) or remove them ?

Excellent. Please remove the rules since they're now not needed.

[st--]

Will do.

What do you think of https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/pull/313/commits/f1df8b501c0c0c624518b53f7fe0eb89a6839cc2 ?

[st--]

Please remove the rules since they're now not needed.

I'm wondering if they might make it a bit faster though (as they're shortcutting some backprop)?

[willtebbutt]

I'm wondering if they might make it a bit faster though (as they're shortcutting some backprop)?

They really shouldn't make stuff faster in this case -- it's simple enough that Zygote ought to be able to infer this properly. In my view, the additional code complexity probably isn't worth it.

[willtebbutt]

What do you think of https://github.com/JuliaGaussianProcesses/AbstractGPs.jl/commit/f1df8b501c0c0c624518b53f7fe0eb89a6839cc2 ?

I like it. Nice to remove some code duplication.