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

Implement cov_diag and mean_and_cov_diag for FiniteGPs #116

Merged
merged 9 commits into from
Mar 29, 2021

Conversation

devmotion
Copy link
Member

@devmotion devmotion commented Mar 29, 2021

This PR

  • implements cov_diag and mean_and_cov_diag for FiniteGPs and uses it to compute marginals
  • adds default implementations of cov_diag (see below), mean_and_cov, and mean_and_cov_diag
  • exports mean_and_cov_diag

@willtebbutt
Copy link
Member

I like this.

Before properly reviewing, the only thing that concerns me in this PR is the addition of the default for cov_diag. In my experience, I would much rather cov_diag fails if I've not got an efficient way to do it so as to avoid hitting a fallback with asymptotically worse complexity than what I would usually expect to get.

Is there a particular reason that you want this fallback implementation?

@devmotion
Copy link
Member Author

No, there's no particular reason apart from the fact that it could be added and it would reduce the number of methods one has to implement for a subtype of AbstractGP. However, I was also debating if it would be better to add default implementations only for mean_and_cov and mean_and_cov_diag such that it obvious that it uses the slow fallback. On the other hand, the default implementation of mean_and_cov and mean_and_cov_diag (and the existing implementation for GP) also use the slow fallback and it's not obvious to users that this is the case (well, at least not for GP).

@devmotion devmotion changed the title Implement mean_and_cov_diag for FiniteGPs Implement cov_diag and mean_and_cov_diag for FiniteGPs Mar 29, 2021
@willtebbutt
Copy link
Member

On the other hand, the default implementation of mean_and_cov and mean_and_cov_diag (and the existing implementation for GP) also use the slow fallback and it's not obvious to users that this is the case (well, at least not for GP).

Right, yes. For me, the distinction is that the default for mean_and_cov and mean_and_cov_diag is (roughly speaking) at worst 2x slower than a fast method. Conversely, the default implementation for cov_diag is potentially orders of magnitude slower.

@devmotion
Copy link
Member Author

True, I'll remove it and if needed it could be added at some point.

@codecov-io
Copy link

Codecov Report

Merging #116 (0f1c268) into master (fbc0213) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
+ Coverage   99.20%   99.23%   +0.02%     
==========================================
  Files           8        9       +1     
  Lines         252      261       +9     
==========================================
+ Hits          250      259       +9     
  Misses          2        2              
Impacted Files Coverage Δ
src/gp/gp.jl 100.00% <ø> (ø)
src/abstract_gp/abstract_gp.jl 100.00% <100.00%> (ø)
src/abstract_gp/finite_gp.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 fbc0213...0f1c268. Read the comment docs.

Copy link
Member

@willtebbutt willtebbutt left a comment

Choose a reason for hiding this comment

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

LGTM

@devmotion devmotion merged commit b78f6dd into master Mar 29, 2021
@devmotion devmotion deleted the dw/mean_and_cov_diag branch March 29, 2021 13:28
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.

3 participants