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

Add ApproximatePeriodicKernel #98

Merged
merged 30 commits into from
May 9, 2023
Merged

Add ApproximatePeriodicKernel #98

merged 30 commits into from
May 9, 2023

Conversation

theogf
Copy link
Member

@theogf theogf commented Mar 21, 2023

This is an attempt at #61

Left todo:

  • Add tests for the ApproxPeriodicKernel

@theogf
Copy link
Member Author

theogf commented Mar 28, 2023

Will be splitted to treat the KernelSum first.

@theogf
Copy link
Member Author

theogf commented Apr 4, 2023

Work in progress:

  • The basic functionality is implemented with the CosineKernel
  • However the logpdf prediction seems to fail, which indicates there might be an error somewhere in the conversion of the kernel to state-space, needs to investigate

@theogf theogf changed the title Add ApproximatePeriodicKernel and sum of multiple kernels Add ApproximatePeriodicKernel Apr 11, 2023
@theogf
Copy link
Member Author

theogf commented Apr 11, 2023

This will need some fixes from #100 but ready otherwise

@theogf theogf marked this pull request as ready for review April 11, 2023 11:40
@theogf
Copy link
Member Author

theogf commented Apr 11, 2023

Another issue is with the argument for the PeriodicKernel, it is bounded but we do not do any transformation. This is messing up computations when using FiniteDifferences

Copy link
Member

@simsurace simsurace left a comment

Choose a reason for hiding this comment

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

Looks clean. Maybe a few tests comparing a normal GP to a temporal GP with ApproxPeriodixKernel would add some safety, e.g. comparing the logpdf output and maybe posterior mean and variance.

src/gp/lti_sde.jl Outdated Show resolved Hide resolved
@theogf
Copy link
Member Author

theogf commented Apr 11, 2023

Looks clean. Maybe a few tests comparing a normal GP to a temporal GP with ApproxPeriodixKernel would add some safety, e.g. comparing the logpdf output and maybe posterior mean and variance.

This is already the case! If you look at the lti_sde tests, the prior is compared against a "naive" GP.

Co-authored-by: Simone Carlo Surace <51025924+simsurace@users.noreply.github.com>
@simsurace
Copy link
Member

simsurace commented Apr 11, 2023

This is already the case! If you look at the lti_sde tests, the prior is compared against a "naive" GP.

Indeed, thanks!

@coveralls
Copy link

coveralls commented Apr 25, 2023

Pull Request Test Coverage Report for Build 4817610971

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 55 of 59 (93.22%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 87.977%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/models/lgssm.jl 0 1 0.0%
src/gp/lti_sde.jl 54 57 94.74%
Totals Coverage Status
Change from base Build 4817383865: 0.3%
Covered Lines: 1361
Relevant Lines: 1547

💛 - Coveralls

@theogf
Copy link
Member Author

theogf commented Apr 25, 2023

@willtebbutt this is fully ready for a review.

@simsurace
Copy link
Member

@willtebbutt are there any blockers for this?

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.

Nope, no blockers really. I'm happy for this to be merged

@theogf theogf merged commit a931ac0 into master May 9, 2023
@theogf theogf deleted the tgf/flexible-kernel branch May 9, 2023 15:05
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.

4 participants