search

JuliaGaussianProcesses / KernelFunctions.jl

Julia package for kernel functions for machine learning
https://juliagaussianprocesses.github.io/KernelFunctions.jl/stable/
MIT License
267 stars 32 forks source link

Fix periodic kernel AD #531

Closed simsurace closed 6 months ago

simsurace commented 10 months ago

This PR reactivates the AD tests for the periodic kernel and introduces rrules to make them pass. Fixes #527

codecov[bot] commented 10 months ago

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (3b7a2df) 90.34% compared to head (774ac73) 67.81%.

Files Patch % Lines
src/chainrules.jl 80.95% 16 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #531 +/- ## =========================================== - Coverage 90.34% 67.81% -22.54% =========================================== Files 52 52 Lines 1378 1454 +76 =========================================== - Hits 1245 986 -259 - Misses 133 468 +335 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

simsurace commented 10 months ago

Ok, I added ChainRulesTestUtils tests for normal and static arrays. The static case is certainly not optimal, but I don't have the capacity to think of how to rewrite the rules to be non-mutating.

If anyone knows off-hand how to do this, feel free to make a suggestion.

simsurace commented 6 months ago

@willtebbutt Could we merge this? The comment regarding the generality of the rule does not I apply (I believe) because of the concrete type of the field of Sinus. The failures are unrelated, see #526.

willtebbutt commented 6 months ago

I have no objection to merging. @devmotion do you have thoughts?