Skip to content

Conversation

@matt-graham
Copy link
Member

Fixes #243

Updates GaussianRandomFields compatible version specifier to v2.2.1 in tests/Project.toml to take advantage of fix for PieterjanRobbe/GaussianRandomFields.jl#45 and removes manual fix for adjusting scaling of covariance in tests/models/llw2d.jl test model.

Also adds instance of linear gaussian model to models for which unit tests for model interface are run for, and adds a test which validates the particle filter estimates against ground truth values computed using a Kalman filter for a linear Gaussian model, based on the existing Pluto notebook for producing plots for visual validation in extras/linear_gaussian_validation.jl.

@matt-graham matt-graham requested a review from DanGiles May 24, 2023 14:14
@codecov-commenter
Copy link

codecov-commenter commented May 24, 2023

Codecov Report

Merging #244 (aabb7c5) into master (87c0324) will increase coverage by 1.06%.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #244      +/-   ##
==========================================
+ Coverage   91.22%   92.28%   +1.06%     
==========================================
  Files           7        7              
  Lines         376      376              
==========================================
+ Hits          343      347       +4     
+ Misses         33       29       -4     

see 1 file with indirect coverage changes

@giordano
Copy link
Member

Why did aabb7c5 (#244) solve the test failures in the multithreaded CI jobs?

Also, can the new tests be made silent, to avoid all the

Writing output at timestep = N

messages?

@matt-graham
Copy link
Member Author

Why did aabb7c5 (#244) solve the test failures in the multithreaded CI jobs?

Sorry yeah was just about post an explanation here as I realised I hadn't followed up after pushing yesterday 😅. The test which which failed was checking the estimates of the variance of the filtering distributions matched ground truth values computed using a Kalman filter, with the specific test failure when running with an ensemble of 10 particles using 2 threads with the bootstrap filter. On doing some local testing with different seeds I found that we also sometimes got failures when running single threaded for this test, with the underlying issue seeming to be that 10 particles is too small an ensemble to avoid the ensemble occasionally collapsing when using a bootstrap filter, resulting in very high variance in the variance estimates (too many variances there...). Increasing the smallest number of particles the test is run over to 30 seemed to be sufficient to avoid these issues and get robust performance over different random realisations.

Also, can the new tests be made silent, to avoid all the

Writing output at timestep = N

messages?

Agree that these are annoying - I would personally prefer to remove all the println statements altogether or at least make them optional with a quiet flag or similar as there isn't any easy way to selectively disable at the moment that I can see. Or alternatively it looks like using macros from the Logging module would be better as then we can selectively disable output for different levels. But this seems a change that would probably be better in a separate PR.

@DanGiles DanGiles merged commit a0e7ea7 into master May 31, 2023
@giordano giordano deleted the mmg/grf-update-and-cov-fix branch May 31, 2023 09:20
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.

Update GaussianRandomFields version in test dependencies

5 participants