Remove `_get_ν` #452

devmotion · 2022-04-13T13:10:11Z

Fix the bug introduced in #425 (see #450).

Removes the workaround.

Co-authored-by: willtebbutt <[email protected]>

src/basekernels/matern.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

theogf · 2022-04-13T13:13:32Z

Is it possible to test this?

codecov · 2022-04-13T13:17:40Z

Codecov Report

Merging #452 (4c45fe8) into master (35de8d2) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

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

Impacted Files	Coverage Δ
src/TestUtils.jl	`94.11% <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 35de8d2...4c45fe8. Read the comment docs.

devmotion · 2022-04-13T13:53:38Z

The rule was wrong, so it seems it's not covered by the tests? Why was this workaround introduced?

devmotion · 2022-04-13T14:10:29Z

It seems in #425 the commented out test_ADs(x->MaternKernel(nu=first(x)),[ν]) was replaced with test_ADs(() -> MaternKernel(; nu=ν)). Was this intended? And could this explain why the workaround is not tested?

st-- · 2022-04-13T14:24:18Z

It seems in #425 the commented out test_ADs(x->MaternKernel(nu=first(x)),[ν]) was replaced with test_ADs(() -> MaternKernel(; nu=ν)). Was this intended? And could this explain why the workaround is not tested?

Yes, that was intended, as discussed on that PR (#425 (comment)), where @theogf wrote

I am proposing that we just ban differentiation through \nu and stop testing for it.

And while the change

-test_ADs(x->MaternKernel(nu=first(x)),[ν])
+test_ADs(() -> MaternKernel(; nu=ν))

worked for Forward- and ReverseDiff, the additional workaround in #425 was required for Zygote to also pass the AD test (w.r.t. the input features, not the kernel hyperparameters!). So the workaround is tested by virtue of the Zygote AD test on MaternKernel now also passing.

devmotion · 2022-04-13T14:26:47Z

So the workaround is tested by virtue of the Zygote AD test on MaternKernel now also passing.

The main problem is that tests passed successfully for 08437cc even though the pullback is clearly incorrect. So I was wondering if the pullback was evaluated at all.

st-- · 2022-04-13T14:28:05Z

src/basekernels/matern.jl

@@ -37,8 +37,18 @@ MaternKernel(; nu::Real=1.5, ν::Real=nu, metric=Euclidean()) = MaternKernel(ν,

 @functor MaternKernel

+# workaround for Zygote
+# unclear why it's needed but it is fine since it's stated officially that we don't support differentiation with respect to ν


Without this workaround, the Zygote AD test fails. You can see this in 30385ac and the corresponding build

st-- · 2022-04-13T14:29:47Z

The main problem is that tests passed successfully for 08437cc even though the pullback is clearly incorrect.

What was incorrect about the pullback?

So I was wondering if the pullback was evaluated at all.

It was, I only added it because without Zygote errored, see #452 (comment)

devmotion · 2022-04-13T14:33:24Z

What was incorrect about the pullback?

It returned the wrong number of outputs (was missing NoTangent() derivative for the function itself).

Without this workaround, the Zygote AD test fails. You can see this in 30385ac and the corresponding build

The problem is just that https://github.com/FluxML/Zygote.jl/blob/95e10218f0187a839ac0539fefcbf39c727bf068/src/lib/array.jl#L30 does not support NotImplemented. This should have been fixed in Zygote, introducing workarounds in KernelFunctions is the wrong approach.

st-- · 2022-04-13T14:37:03Z

This should have been fixed in Zygote, introducing workarounds in KernelFunctions is the wrong approach.

Yeah, I completely agree, but unfortunately it hasn't been fixed in Zygote, and I think "workaround + AD is working" is still preferable over "no workaround + AD is broken".

devmotion · 2022-04-13T14:38:18Z

Instead of making a PR to KernelFunctions introducing a workaround you could have made one for Zygote that fixes the issue 🤔

st-- · 2022-04-13T14:43:24Z

Instead of making a PR to KernelFunctions introducing a workaround you could have made one for Zygote that fixes the issue 🤔

I managed to figure out how to fix the issue with a workaround in KernelFunctions. That doesn't mean I would immediately know how to identify & fix the underlying issue upstream. If I did know how to fix it there, I would have, as I have in various other cases. You seem to have an amazing overview as well as deep understanding of a lot of different packages. The ability and time for that is unfortunately not something that everyone has available in the same amount. I'm assuming you're not intending to, but you give the impression of being rather disparaging of other people who're not thinking the same way you are.

test/basekernels/matern.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

devmotion · 2022-04-13T14:48:00Z

IMO usually it is very helpful to open at least Github issues in cases where it is not immediately clear how to fix the issue. Workarounds can be acceptable if they are fixed upstream eventually, but I assume it is unlikely that they are fixed if nobody makes the Zygote developers aware of it.

Generally, I would have given you this advice in your PR but as desired I don't review your PRs anymore.

test/basekernels/matern.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

st-- · 2022-04-13T19:58:52Z

@devmotion I've pared it down to a minimal reproducible example: FluxML/Zygote.jl#1204 but I can't comment on anything else, if you've got further thoughts on what the issue is / what's needed to fix it, please comment there.

devmotion · 2022-04-14T00:18:43Z

Test errors seem unrelated? I didn't change anything related to them, I think.

willtebbutt · 2022-04-14T07:15:42Z

Test errors seem unrelated? I didn't change anything related to them, I think.

Agreed. Maybe we need to relax a tolerance somewhere?

st-- · 2022-04-14T07:27:40Z

Test errors seem unrelated? I didn't change anything related to them, I think.

Agreed. Maybe we need to relax a tolerance somewhere?

Yes, I think it's due to the changes in #447; the tests are stochastic (if you just re-run the failed tests, they might pass the next time - I've seen that in #454 as well which has zero code changes). In #447 I reduced the size of the kernel matrix evaluations; the isapprox() call (set with atol=1e-9, rtol=1e-9) is on the arrays as a whole, which does not look at individual elements, but instead checks whether the norm of the difference is within the tolerances, relative to the max of the norm of the individual matrices (see discussion). For the random inputs we're using in the tests, the norm of the kernel matrix is linear in the dimensions. So by reducing the size from ~1000 to ~10 we're reducing the norm by a factor ~100, so yes that suggests to me we should increase the rtol correspondingly.

Alternatively, we could change it to element-wise isapprox comparison? then it's independent of the matrix norm.

devmotion · 2022-04-14T09:42:53Z

Shouldn't the scale be irrelevant if we use rtol? I think it's problematic that we don't use rtol in most of the tests but only in

KernelFunctions.jl/src/test_utils.jl

Lines 75 to 76 in 8e805ef

    
           @test kernelmatrix_diag(k, x0) ≈ kernelmatrix_diag(k, x0, x0) atol = atol rtol = rtol 
        
           @test kernelmatrix(k, x0) ≈ kernelmatrix(k, x0, x0) atol = atol rtol = rtol

. Probably because otherwise tests failed - which IMO, however, would not indicate a problem with rtol per se but that the default already was already too low even before the recent changes. I completely agree with https://discourse.julialang.org/t/unit-test-equality-for-arrays/8138/12?u=devmotion that in general you want rtol since it is completely unclear what a meaningful atol is in general. Specifying a small atol as well mainly helps with comparisons that involve exact 0s which would fail otherwise.

st-- · 2022-04-14T10:50:07Z

I think it's problematic that we don't use rtol in most of the tests

Yes, I agree. Probably an oversight in the original test code of not realizing that by only setting atol you end up setting rtol=0...

devmotion · 2022-04-19T17:40:35Z

Apparently #455 was not sufficient.

devmotion · 2022-05-01T23:12:58Z

Bump.

devmotion · 2022-05-08T19:40:04Z

Bump.

theogf · 2022-05-08T21:18:42Z

Could we keep the global atol and rtol variables?

devmotion · 2022-05-08T22:19:28Z

I don't mind. It just seemed unnecessary to have global variables that are used only in a single place, and clearer (ie easier to spot) if the values are just used as default keyword arguments in the function definition.

devmotion · 2022-05-16T13:25:05Z

@theogf Did you see my last comment? Would you like it to be reverted?

theogf · 2022-05-17T10:54:15Z

I think keeping the global const is a bit more elegant and easily findable since it's in the same file. But again I do not mind, I'll just accept the PR and do what you want 😅

devmotion and others added 2 commits April 13, 2022 15:08

Fix derivative of _get_ν

c9015fa

Co-authored-by: willtebbutt <[email protected]>

Update Project.toml

c4d23e8

github-actions bot reviewed Apr 13, 2022

View reviewed changes

src/basekernels/matern.jl Outdated Show resolved Hide resolved

Update src/basekernels/matern.jl

08437cc

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Fix rrule

8e4ad8f

Add test

9bb7beb

Load ChainRulesTestUtils

6a467f9

st-- reviewed Apr 13, 2022

View reviewed changes

Try to specify cotangent

b95a84a

github-actions bot reviewed Apr 13, 2022

View reviewed changes

test/basekernels/matern.jl Outdated Show resolved Hide resolved

Update test/basekernels/matern.jl

c747f70

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Update matern.jl

aadcfa8

github-actions bot reviewed Apr 13, 2022

View reviewed changes

test/basekernels/matern.jl Outdated Show resolved Hide resolved

Update test/basekernels/matern.jl

d87cc84

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

devmotion mentioned this pull request Apr 13, 2022

AD error due to interaction of missing gradient of besselk and struct with vector FluxML/Zygote.jl#1204

Closed

Remove workaround

e7f658b

devmotion changed the title ~~Fix derivative of _get_ν~~ Remove _get_ν Apr 14, 2022

devmotion added 2 commits April 19, 2022 18:54

Update Project.toml

8473386

Merge branch 'master' into dw/fix_matern

5b4e9d3

Fix test_interface

4c45fe8

devmotion requested a review from theogf April 19, 2022 18:19

theogf approved these changes May 17, 2022

View reviewed changes

devmotion merged commit e0a7af6 into master May 17, 2022

devmotion deleted the dw/fix_matern branch May 17, 2022 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `_get_ν` #452

Remove `_get_ν` #452

devmotion commented Apr 13, 2022 •

edited

Loading

theogf commented Apr 13, 2022

codecov bot commented Apr 13, 2022 •

edited

Loading

devmotion commented Apr 13, 2022

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022 •

edited

Loading

devmotion commented Apr 13, 2022

st-- Apr 13, 2022 •

edited

Loading

st-- commented Apr 13, 2022

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022 •

edited

Loading

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022

devmotion commented Apr 14, 2022

willtebbutt commented Apr 14, 2022

st-- commented Apr 14, 2022 •

edited

Loading

devmotion commented Apr 14, 2022

st-- commented Apr 14, 2022

devmotion commented Apr 19, 2022

devmotion commented May 1, 2022

devmotion commented May 8, 2022

theogf commented May 8, 2022

devmotion commented May 8, 2022

devmotion commented May 16, 2022

theogf commented May 17, 2022

Remove _get_ν #452

Remove _get_ν #452

Conversation

devmotion commented Apr 13, 2022 • edited Loading

theogf commented Apr 13, 2022

codecov bot commented Apr 13, 2022 • edited Loading

Codecov Report

devmotion commented Apr 13, 2022

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022 • edited Loading

devmotion commented Apr 13, 2022

st-- Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

st-- commented Apr 13, 2022

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022 • edited Loading

devmotion commented Apr 13, 2022

st-- commented Apr 13, 2022

devmotion commented Apr 14, 2022

willtebbutt commented Apr 14, 2022

st-- commented Apr 14, 2022 • edited Loading

devmotion commented Apr 14, 2022

st-- commented Apr 14, 2022

devmotion commented Apr 19, 2022

devmotion commented May 1, 2022

devmotion commented May 8, 2022

theogf commented May 8, 2022

devmotion commented May 8, 2022

devmotion commented May 16, 2022

theogf commented May 17, 2022

Remove `_get_ν` #452

Remove `_get_ν` #452

devmotion commented Apr 13, 2022 •

edited

Loading

codecov bot commented Apr 13, 2022 •

edited

Loading

st-- commented Apr 13, 2022 •

edited

Loading

st-- Apr 13, 2022 •

edited

Loading

st-- commented Apr 13, 2022 •

edited

Loading

st-- commented Apr 14, 2022 •

edited

Loading