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

MOInput Type Stability #465

Closed willtebbutt closed 2 years ago

willtebbutt commented 2 years ago

Summary Makes the type of out_dim field of MOInput a type parameter of MOInput. This has been done in a non-breaking way.

Proposed changes

See summary

What alternatives have you considered?

https://github.com/JuliaGaussianProcesses/KernelFunctions.jl/pull/405 consides an alternative approach in which the type is constrained to Int. This has the disadvantage of being more constrained, and constituting a break change. The potential advantage over the approach used here is slight changes in compile time, but my instinct is that these will be negligible.

Breaking changes

None

theogf commented 2 years ago

Maybe add a @inferred test?

codecov[bot] commented 2 years ago

Codecov Report

Merging #465 (f1f181b) into master (bda66f8) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #465   +/-   ##
=======================================
  Coverage   93.16%   93.16%           
=======================================
  Files          52       52           
  Lines        1259     1259           
=======================================
  Hits         1173     1173           
  Misses         86       86
Impacted Files Coverage Δ
src/mokernels/moinput.jl 100.00% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

willtebbutt commented 2 years ago

@theogf done!

willtebbutt commented 2 years ago

Thanks for the swift review @theogf -- will merge when CI passes

willtebbutt commented 2 years ago

@theogf looks like Others on 1.3 is failing on the doctests -- the formatting of the type printing appears to be different for that version. I don't know how tests were passing previously, because presumably this hasn't changed... 😬

Any thoughts?

theogf commented 2 years ago

Probably the doctestfilter don't catch so many types or something? Didn't we mention dropping 1.3 anyways?

willtebbutt commented 2 years ago

Cool. I'm just going to drop CI for 1.3

willtebbutt commented 2 years ago

@theogf any idea how I tell github to not require the 1.3 tests to be run?

Nvm, figured it out :)