Zygote AD failure workarounds & test cleanup #414

st-- · 2021-12-09T15:10:49Z

Zygote seems to have changed again and we now need to revert #409

Edit: there is now a new AD failure in the select transform (#415), so temporarily disabled that test case.

In the process of trying (and failing) to figure out what's going on, I noticed that there was a lot of code duplication in test/test_utils which I've removed here (left comments to explain what I changed).

codecov · 2021-12-09T15:16:18Z

Codecov Report

Merging #414 (8b1cddc) into master (3c49949) will increase coverage by 3.50%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #414      +/-   ##
==========================================
+ Coverage   89.23%   92.73%   +3.50%     
==========================================
  Files          52       52              
  Lines        1198     1198              
==========================================
+ Hits         1069     1111      +42     
+ Misses        129       87      -42

Impacted Files	Coverage Δ
src/zygoterules.jl	`33.33% <0.00%> (-16.67%)`	⬇️
src/basekernels/fbm.jl	`100.00% <0.00%> (+2.38%)`	⬆️
src/basekernels/exponential.jl	`100.00% <0.00%> (+3.84%)`	⬆️
src/test_utils.jl	`94.11% <0.00%> (+3.92%)`	⬆️
src/basekernels/matern.jl	`100.00% <0.00%> (+4.00%)`	⬆️
src/transform/chaintransform.jl	`78.26% <0.00%> (+4.34%)`	⬆️
src/mokernels/slfm.jl	`100.00% <0.00%> (+4.76%)`	⬆️
src/kernels/kerneltensorproduct.jl	`98.76% <0.00%> (+4.93%)`	⬆️
src/basekernels/sm.jl	`100.00% <0.00%> (+5.88%)`	⬆️
src/mokernels/independent.jl	`100.00% <0.00%> (+6.25%)`	⬆️
... and 10 more

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 3c49949...8b1cddc. Read the comment docs.

st-- · 2021-12-16T14:26:49Z

test/test_utils.jl

+                if args !== nothing
+                    compare_gradient(AD, args) do p
+                        testdiagfunction(kernelfunction(p), A, dim)
+                    end
                end
            end
        end


@rossviljoen doing the changes above I spotted that you overloaded test_AD() for MOKernel - this worries me: it seems like all the "fallback" AD tests above are not run. It also seems to suggest that we can't substitute an MOKernel where we need an arbitrary Kernel (which would I believe violate Liskov substitution principle). Could you comment pls? thanks:)

I don't think that was me?

Ah, apologies! Now I'm not even sure anymore why I assumed it was you 😔 @willtebbutt / @thomasgudjonwright ?

Hmm don't think it was me. A lot of this code was @theogf I think?

I did the original compare gradient but someone else extended it to MOKernel

Wasn't me! Could always look back at the commit history :)

Should've checked git blame in the first place... it was #263 (@david-vicente).

This to me seems like a case where overloading the same method isn't really the right thing to do (because it suggests they do the same thing - it's a kernel AD check in either case - but they don't actually seem to check the same things)... now that i've got y'all in a thread, what is your opinion on these ?

Separated out into #416

test/transform/chaintransform.jl

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

…ernelFunctions.jl into st/zygote_again

st-- · 2021-12-18T19:18:45Z

test/test_utils.jl

@@ -114,70 +118,6 @@ function test_ADs(
    end
 end

-function test_FiniteDiff(kernelfunction, args=nothing, dims=[3, 3])


This was doing exactly the same as test_AD (except for testdiagfunction(kernelfunction(p), A, dim) vs testdiagfunction(kernelfunction(p), A, B, dim), which I've now added to test_AD). The only difference was calling compare_gradient vs the @test_warn, so I simply added a compare_gradient(f, ::Val{:FiniteDiff}, args) so we can avoid all this code duplication.

test/test_utils.jl

st-- · 2021-12-18T19:26:24Z

test/test_utils.jl

-            compare_gradient(AD, A) do a
-                testdiagfunction(k, a, dim)
-            end
-            compare_gradient(AD, A) do a
-                testdiagfunction(k, a, B, dim)
-            end
-            compare_gradient(AD, B) do b
-                testdiagfunction(k, A, b, dim)
-            end
-            if args !== nothing
-                compare_gradient(AD, args) do p
-                    testdiagfunction(kernelfunction(p), A, dim)


this was doing exactly the same for testdiagfunction as the code above for testfunction, so I've unified it with a for loop over the two functions.

st-- · 2021-12-18T19:26:40Z

test/test_utils.jl

+                                _testfn(kernelfunction(p), A, dim)
+                            end
+                            compare_gradient(AD, args) do p
+                                _testfn(kernelfunction(p), A, B, dim)


added missing case

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

willtebbutt

I'm happy for this to go in. Thanks for sorting this out @st--

commit f9bbd84 Author: st-- <[email protected]> Date: Fri Jan 28 09:11:50 2022 +0100 make nystrom work with AbstractVector (#427) * make nystrom work with AbstractVector * add test * Update test/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * patch bump * Update test/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review * deprecate * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: Théo Galy-Fajou <[email protected]> * Update src/approximations/nystrom.jl Co-authored-by: Théo Galy-Fajou <[email protected]> * Update src/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> commit d1c68a9 Author: st-- <[email protected]> Date: Thu Jan 13 22:33:43 2022 +0100 fix Distances compat (#423) * CompatHelper: bump compat for Distances to 0.10 for package test, (keep existing compat) * try out Theo's fix * fix test compat * use ForwardDiff for chain rule test of SqMahalanobis * test on 1.4 instead of 1.3 - see if the chainrules test passes there * revert version branch * revert to 1.3 * test_broken for older Julia versions Co-authored-by: CompatHelper Julia <[email protected]> commit 93d33c2 Author: st-- <[email protected]> Date: Wed Jan 12 14:11:14 2022 +0100 fix figure & cleanup (#422) * fix figure & cleanup * bump LIBSVM compat & Manifest * improve writing, replaces #321 commit 40cb59e Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed Jan 12 09:39:01 2022 +0200 CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) (#367) * CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) * ] up Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: st-- <[email protected]> commit 7204529 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue Jan 11 18:37:23 2022 +0200 CompatHelper: bump compat for Kronecker to 0.5 for package test, (keep existing compat) (#366) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: st-- <[email protected]> commit 924925d Author: st-- <[email protected]> Date: Tue Jan 11 16:26:02 2022 +0100 switch SVM example to half-moon dataset (#421) commit 992b665 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri Dec 24 12:18:56 2021 +0200 CompatHelper: bump compat for SpecialFunctions to 2 for package test, (keep existing compat) (#412) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> Co-authored-by: st-- <[email protected]> commit 04fa7f7 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu Dec 23 13:33:59 2021 +0200 CompatHelper: bump compat for SpecialFunctions to 2, (keep existing compat) (#411) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> Co-authored-by: st-- <[email protected]> commit c0fc3e1 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu Dec 23 01:10:40 2021 +0200 CompatHelper: add new compat entry for Compat at version 3 for package test, (keep existing compat) (#418) Co-authored-by: CompatHelper Julia <[email protected]> commit 05fe340 Author: st-- <[email protected]> Date: Tue Dec 21 00:49:37 2021 +0200 use only() instead of first() (#403) * use only() instead of first() for 1-"vectors" that were for the benefit of Flux * fix one test that should not have worked as it was * add missing scalar Sinus constructor commit 2d17212 Author: st-- <[email protected]> Date: Sat Dec 18 23:43:30 2021 +0200 Zygote AD failure workarounds & test cleanup (#414) Zygote AD failures: * revert #409 (test_utils workaround for broken Zygote - now working again) * disable broken Zygote AD test for ChainTransform Improved tests: * finer-grained testsets * add missing test cases to test_AD * replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...) * remove code duplication commit 3c49949 Author: Théo Galy-Fajou <[email protected]> Date: Wed Nov 24 18:32:19 2021 +0100 Fix typo in valid_inputs error (#408) * Fix typo in valid_inputs error * Update src/utils.jl Co-authored-by: David Widmann <[email protected]> Co-authored-by: David Widmann <[email protected]> commit 9955044 Author: st-- <[email protected]> Date: Wed Nov 24 18:55:18 2021 +0200 Fix for Zygote 0.6.30 breaking our tests (#409) * restrict Zygote to <0.6.30 * revert Zygote test restriction and add finer-grained testset * Update test/utils.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * revert testset * mark test_broken * Use `@test_throws` instead of `@test_broken` Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> commit 33d64d1 Author: Théo Galy-Fajou <[email protected]> Date: Thu Nov 4 14:23:57 2021 +0100 Add benchmarking CI (#399) * Add benchmark file * delete old benchmarks * Add github action * Add Project * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Missing end of line Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 360ce10 Author: David Widmann <[email protected]> Date: Tue Nov 2 11:09:58 2021 +0100 Update docstring of `GibbsKernel` (#395)

* initial version of training kernel parameters example from st/examples (#234) * update script * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix out-of-domain initial value * initial version of training kernel parameters example from st/examples (#234) * update script * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fix out-of-domain initial value * Add ParameterHandling * Extend example * Failed attempts with default Flux opt * Add Flux.destructure example * Add some nothing * Squashed commit of the following: commit f9bbd84 Author: st-- <[email protected]> Date: Fri Jan 28 09:11:50 2022 +0100 make nystrom work with AbstractVector (#427) * make nystrom work with AbstractVector * add test * Update test/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * patch bump * Update test/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review * deprecate * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: Théo Galy-Fajou <[email protected]> * Update src/approximations/nystrom.jl Co-authored-by: Théo Galy-Fajou <[email protected]> * Update src/approximations/nystrom.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> commit d1c68a9 Author: st-- <[email protected]> Date: Thu Jan 13 22:33:43 2022 +0100 fix Distances compat (#423) * CompatHelper: bump compat for Distances to 0.10 for package test, (keep existing compat) * try out Theo's fix * fix test compat * use ForwardDiff for chain rule test of SqMahalanobis * test on 1.4 instead of 1.3 - see if the chainrules test passes there * revert version branch * revert to 1.3 * test_broken for older Julia versions Co-authored-by: CompatHelper Julia <[email protected]> commit 93d33c2 Author: st-- <[email protected]> Date: Wed Jan 12 14:11:14 2022 +0100 fix figure & cleanup (#422) * fix figure & cleanup * bump LIBSVM compat & Manifest * improve writing, replaces #321 commit 40cb59e Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed Jan 12 09:39:01 2022 +0200 CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) (#367) * CompatHelper: bump compat for Kronecker to 0.5 for package docs, (keep existing compat) * ] up Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: st-- <[email protected]> commit 7204529 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue Jan 11 18:37:23 2022 +0200 CompatHelper: bump compat for Kronecker to 0.5 for package test, (keep existing compat) (#366) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: st-- <[email protected]> commit 924925d Author: st-- <[email protected]> Date: Tue Jan 11 16:26:02 2022 +0100 switch SVM example to half-moon dataset (#421) commit 992b665 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri Dec 24 12:18:56 2021 +0200 CompatHelper: bump compat for SpecialFunctions to 2 for package test, (keep existing compat) (#412) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> Co-authored-by: st-- <[email protected]> commit 04fa7f7 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu Dec 23 13:33:59 2021 +0200 CompatHelper: bump compat for SpecialFunctions to 2, (keep existing compat) (#411) Co-authored-by: CompatHelper Julia <[email protected]> Co-authored-by: Théo Galy-Fajou <[email protected]> Co-authored-by: st-- <[email protected]> commit c0fc3e1 Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu Dec 23 01:10:40 2021 +0200 CompatHelper: add new compat entry for Compat at version 3 for package test, (keep existing compat) (#418) Co-authored-by: CompatHelper Julia <[email protected]> commit 05fe340 Author: st-- <[email protected]> Date: Tue Dec 21 00:49:37 2021 +0200 use only() instead of first() (#403) * use only() instead of first() for 1-"vectors" that were for the benefit of Flux * fix one test that should not have worked as it was * add missing scalar Sinus constructor commit 2d17212 Author: st-- <[email protected]> Date: Sat Dec 18 23:43:30 2021 +0200 Zygote AD failure workarounds & test cleanup (#414) Zygote AD failures: * revert #409 (test_utils workaround for broken Zygote - now working again) * disable broken Zygote AD test for ChainTransform Improved tests: * finer-grained testsets * add missing test cases to test_AD * replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...) * remove code duplication commit 3c49949 Author: Théo Galy-Fajou <[email protected]> Date: Wed Nov 24 18:32:19 2021 +0100 Fix typo in valid_inputs error (#408) * Fix typo in valid_inputs error * Update src/utils.jl Co-authored-by: David Widmann <[email protected]> Co-authored-by: David Widmann <[email protected]> commit 9955044 Author: st-- <[email protected]> Date: Wed Nov 24 18:55:18 2021 +0200 Fix for Zygote 0.6.30 breaking our tests (#409) * restrict Zygote to <0.6.30 * revert Zygote test restriction and add finer-grained testset * Update test/utils.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * revert testset * mark test_broken * Use `@test_throws` instead of `@test_broken` Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> commit 33d64d1 Author: Théo Galy-Fajou <[email protected]> Date: Thu Nov 4 14:23:57 2021 +0100 Add benchmarking CI (#399) * Add benchmark file * delete old benchmarks * Add github action * Add Project * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Missing end of line Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> commit 360ce10 Author: David Widmann <[email protected]> Date: Tue Nov 2 11:09:58 2021 +0100 Update docstring of `GibbsKernel` (#395) * change title * Update manifest * Formatter * Some small updates * Add BenchmarkTools * Move Benchmarks to the correct manifest * Formatter * Fix missed comment * Add ParameterHandling too * Change gif embed * Forgot # * Formatter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Compat and gif kwargs * , for ; * different commit msg * Some more clarification * Change display * Final changes * Apply suggestions from code review Co-authored-by: st-- <[email protected]> * Apply suggestions from code review Co-authored-by: st-- <[email protected]> * Address comments * Missed one * Manifest issue * Apply suggestions from formatter Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Local formatter * Delete loss description. Co-authored-by: st-- <[email protected]> * format Co-authored-by: ST John <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

revert test_utils workaround for broken Zygote

7ccfbef

st-- changed the title ~~revert test_utils workaround for broken Zygote~~ fix Zygote again Dec 9, 2021

finer-grained testsets

313535e

st-- commented Dec 16, 2021

View reviewed changes

disable broken Zygote AD test for ChainTransform

11b2bcd

github-actions bot reviewed Dec 18, 2021

View reviewed changes

test/transform/chaintransform.jl Outdated Show resolved Hide resolved

st-- and others added 3 commits December 18, 2021 20:46

Apply suggestions from code review

feb5b5c

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

add missing test cases to test_AD

919e445

replace test_FiniteDiff with test_AD(..., :FiniteDiff, ...)

7d38977

st-- mentioned this pull request Dec 18, 2021

Zygote AD failure for ChainTransform #415

Open

more explicit test broken

a4ad008

st-- mentioned this pull request Dec 18, 2021

test_AD "normal" kernels vs MOKernels #416

Open

Merge branch 'st/zygote_again' of github.com:JuliaGaussianProcesses/K…

51c9250

…ernelFunctions.jl into st/zygote_again

st-- commented Dec 18, 2021

View reviewed changes

remove more duplication

821503c

github-actions bot reviewed Dec 18, 2021

View reviewed changes

test/test_utils.jl Outdated Show resolved Hide resolved

st-- commented Dec 18, 2021

View reviewed changes

Update test/test_utils.jl

8b1cddc

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

st-- changed the title ~~fix Zygote again~~ Zygote AD failure workarounds & test cleanup Dec 18, 2021

willtebbutt approved these changes Dec 18, 2021

View reviewed changes

st-- merged commit 2d17212 into master Dec 18, 2021

st-- deleted the st/zygote_again branch December 18, 2021 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Zygote AD failure workarounds & test cleanup #414

Zygote AD failure workarounds & test cleanup #414

st-- commented Dec 9, 2021 •

edited

Loading

codecov bot commented Dec 9, 2021 •

edited

Loading

st-- Dec 16, 2021

rossviljoen Dec 16, 2021

st-- Dec 18, 2021

willtebbutt Dec 18, 2021

theogf Dec 18, 2021

thomasgudjonwright Dec 18, 2021

st-- Dec 18, 2021

st-- Dec 18, 2021

st-- Dec 18, 2021

willtebbutt Dec 18, 2021

st-- Dec 18, 2021

st-- Dec 18, 2021

willtebbutt left a comment

Zygote AD failure workarounds & test cleanup #414

Zygote AD failure workarounds & test cleanup #414

Conversation

st-- commented Dec 9, 2021 • edited Loading

codecov bot commented Dec 9, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willtebbutt left a comment

Choose a reason for hiding this comment

st-- commented Dec 9, 2021 •

edited

Loading

codecov bot commented Dec 9, 2021 •

edited

Loading