Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate implicit obsdim keyword argument and improve tests #264

Merged
merged 9 commits into from Feb 1, 2022

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Jan 31, 2022

Pendant to JuliaGaussianProcesses/KernelFunctions.jl#429.

The PR

  • widens the type annotation of the obsdim keyword argument such that the new default nothing in KernelFunctions works
  • moves the AbstractMatrix handling to the user-facing API such that internally one only has to deal with vectors such as ColVecs and RowVecs
  • improves tests by testing the user-facing API instead of internal constructors
  • adds tests of deprecated implicit obsdim keyword argument

Fixes #257. Alternative to #260.

src/finite_gp_projection.jl Outdated Show resolved Hide resolved
test/finite_gp_projection.jl Outdated Show resolved Hide resolved
test/finite_gp_projection.jl Outdated Show resolved Hide resolved
devmotion and others added 3 commits January 31, 2022 22:16
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #264 (e79cfc8) into master (65bef42) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #264   +/-   ##
=======================================
  Coverage   97.88%   97.88%           
=======================================
  Files          10       10           
  Lines         379      379           
=======================================
  Hits          371      371           
  Misses          8        8           
Impacted Files Coverage Δ
src/finite_gp_projection.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 65bef42...e79cfc8. Read the comment docs.

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@@ -36,6 +30,11 @@ end
Base.length(f::FiniteGP) = length(f.x)

(f::AbstractGP)(x...) = FiniteGP(f, x...)
function (f::AbstractGP)(
X::AbstractMatrix, args...; obsdim::Union{Int,Nothing}=KernelFunctions.defaultobs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why the args... instead of sigma^2 here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To capture all possible arguments (if someone decides to create some other internal FiniteGP constructors) and to not have to worry about default values of some arguments. Similar to what we do in KernelFunctions, with this PR we just forward everything to the vector case and handle arguments, default values, etc. there.

@@ -36,6 +30,11 @@ end
Base.length(f::FiniteGP) = length(f.x)

(f::AbstractGP)(x...) = FiniteGP(f, x...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be

Suggested change
(f::AbstractGP)(x...) = FiniteGP(f, x...)
(f::AbstractGP)(x...; kwargs...) = FiniteGP(f, x...; kwargs...)

or is it done automatically?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it isn't done automatically (and hence one can't change obsdim with the official API - but this is not tested either...). However, since FiniteGP does not accept keyword arguments it is not really needed currently after applying the fixes and dispatches in this PR.

src/finite_gp_projection.jl Outdated Show resolved Hide resolved
@theogf
Copy link
Member

theogf commented Feb 1, 2022

Just need another patch bump

@devmotion
Copy link
Member Author

Done 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GP's call should not accept matrix (without RowVecs/ColVecs)
2 participants