search

JuliaGaussianProcesses / TemporalGPs.jl

Fast inference for Gaussian processes in problems involving time. Partly built on results from https://proceedings.mlr.press/v161/tebbutt21a.html
MIT License
119 stars 5 forks source link

Massive update #90

Closed theogf closed 1 year ago

theogf commented 1 year ago

This:

This PR does not solve the type instabilities and allocations arising from the updates of other packages. These performance problems will be moved into issues of their own to be tackled separately while the rest of the package can be updated and improved.

News!

This ended up being bigger than all of us.

So updated things were:

theogf commented 1 year ago

@willtebbutt It looks like the day has come! There is already a lot of changes and because I am a terrible SE, I of course added more noise with minor changes that came along the way... (MersenneTwister -> Xoshiro, spacing, etc...), so sorry about that.

There might still be some minor issues with CI, I will correct them as they come.

willtebbutt commented 1 year ago

@theogf I'm basically happy with this PR. Is there much else that you want to do, or are you keen to get it merged?

theogf commented 1 year ago

@theogf I'm basically happy with this PR. Is there much else that you want to do, or are you keen to get it merged?

There is still one issue I am trying to resolve (notice that test models-lgssm is failing). It seems that the tangent for the marginals on Reverse mode are not working..

[EDIT:] It should be fixed now.

theogf commented 1 year ago

@theogf I'm basically happy with this PR. Is there much else that you want to do, or are you keen to get it merged?

There is still one issue I am trying to resolve (notice that test models-lgssm is failing). It seems that the tangent for the marginals on Reverse mode are not working..

[EDIT:] It should be fixed now.

Nevermind my fix was not a fix... The problem is that a NoTangent is received when it should not be there... I will try to check this later this week.

simsurace commented 1 year ago

Maybe we could merge the CompatHelper PRs as well before tagging the release?

theogf commented 1 year ago

Maybe we could merge the CompatHelper PRs as well before tagging the release?

I already updated the bounds in this MR. The CompatHelper PRs can be closed once this is in.

theogf commented 1 year ago

Ok I found the issue. When using the interface like rand, logpdf or marginals, we get rid of the state from scan_emit. test_rrule just builds a NoTangent which creates errors downstream.

I partially fixed it by calling scan_emit explicitely. However rand is still a problem as ChainRulesTestUtils has some issues with AbstractRNG objects it seems.

theogf commented 1 year ago

The new error comes from a change in FillArrays, I will try to figure out what's happening.

theogf commented 1 year ago

The new error comes from a change in FillArrays, I will try to figure out what's happening.

This is an issue that has to do with FillArrays, opened one here and fixed the compat for now

theogf commented 1 year ago

I had to remove the tests for the invariant LGSSM, I will add an issue to add them back.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 4480292273

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.

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/models/linear_gaussian_conditionals.jl 6 7 85.71%
src/models/missings.jl 9 10 90.0%
src/util/scan.jl 29 33 87.88%
src/util/chainrules.jl 104 211 49.29%
<!-- Total: 241 354 68.08% -->
Totals Coverage Status
Change from base Build 2238706956: 87.4%
Covered Lines: 1241
Relevant Lines: 1420
💛 - Coveralls