fix MaternKernel AD, but remove differentiation wrt \nu

[st--]

to keep track of whether it's working again

~Now changed so that it only tests Forward/Reverse diff AD, which are working fine at this point.

Not sure why Zygote AD test is failing, or how to fix it :|~

So..... turns out the Zygote issue was the accessing of the kernel object's field. 🤯

Something like this works fine:

using KernelFunctions, Zygote
@inline function matern_kappa_hardcoded_nu(d::Real)
       result = KernelFunctions._matern(1.234, d)
       return ifelse(iszero(d), one(result), result)
end
Zygote.gradient(matern_kappa_hardcoded_nu, 2.345)

So the solution was to simply wrap the k.\nu in a ignore_derivatives... which is fine because the docstring explicitly denies this anyways.

[codecov]

Codecov Report

Merging #425 (8aee1a4) into master (873aa8d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
+ Coverage   93.16%   93.18%   +0.01%     
==========================================
  Files          52       52              
  Lines        1259     1261       +2     
==========================================
+ Hits         1173     1175       +2     
  Misses         86       86              

Impacted Files	Coverage Δ
src/matrix/kernelkroneckermat.jl	100.00% <ø> (ø)
src/basekernels/matern.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 873aa8d...8aee1a4. Read the comment docs.

[devmotion]

It's unlikely that this will be fixed anytime soon since eg for ChainRules derivatives wrt nu are not implemented in SpecialFunctions because they are more involved and would require additional dependencies.

[theogf]

The problem is not differentiating through \nu no? The problem is to differentiate through the Bessel function given the distance input.
It's quite clear that the derivatives given \nu are too ugly and complicated to deal with and we should make it clear that the kernel is not differentiable by \nu

[devmotion]

That's what's tested and commented on in the test though, isn't it?

[theogf]

That's true. That's why I am proposing that we just ban differentiation through \nu and stop testing for it.

[devmotion]

Sounds good to me 🙂

[st--]

that we just ban differentiation through \nu

Would you want to do that explicitly in code? Or just add a comment to the docstring?

[theogf]

Would you want to do that explicitly in code? Or just add a comment to the docstring?

I think adding a comment to the docstring is enough

[st--]

We need to add a @non_differentiable somewhere for Zygote...

[devmotion]

Might be an upstream issue, it seems Zygote tries to convert ChainRulesCore.NotImplemented which by design does not support most operations.

[willtebbutt]

I wonder whether this is quite the right way to handle this.
In particular, what happens if someone does try to compute gradients w.r.t. k.ν? Do they silently get the wrong answer?

I wonder whether it would make more sense to try and return a NotImplemented tangent, see here, which should error if the user ever tries to do anything with the tangent.

[st--]

Yes, this change would mean that there should be a zero gradient wrt k.\nu [NB- I find unicode field names so annoying!]. @theogf suggested previously that a note in the docstring would be enough.

I agree in principle returning an error would be better, but I wouldn't be sure off the top of my head how to actually implement that. Could we leave that for an issue/future PR?

[st--]

NB- I don't know why the field access confuses Zygote. It's not got anything to do with the gradient not being implemented!?

[willtebbutt]

I'm really not sure that I'm comfortable with a silently-incorrect gradient -- if I encountered this as a user, I would be furious 😂 (I think). I'll take a look at how to do this on my machine now and provide a suggestion.

[st--]

if you can fix it right away, that also works:)

[st--]

Personally I'd still prefer a docstring-explained missing gradient wrt one hyperparameter over no gradients working 😄

[willtebbutt]

Haha fair enough. If I can't find a fix, we should merge it.

[willtebbutt]

I'm basically happy with this, please feel free to merge once the commented-out code has been un-commented :)

edit: also, patch bump