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

Move functions for testing models into main package #294

Merged
merged 9 commits into from
Dec 18, 2024

Conversation

matt-graham
Copy link
Member

Currently we have some general purpose functions for testing that models adhere with the expected interface (define the required methods) in tests/runtests.jl, which we use to check the test models in tests/models. These include a function to check model implements interface required for general filters correctly, the additional methods required for using the locally optimal proposal filter and a function to validate filters when applied to linear-Gaussian models against the Kalman filter.

To simplify testing models defined by users or in other repositories such as ParticleDA-UseCases it would be useful for these functions to be available within the main ParticleDA package which is what this PR does.

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.64%. Comparing base (c55ea63) to head (abfc541).
Report is 6 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #294      +/-   ##
==========================================
+ Coverage   92.46%   94.64%   +2.18%     
==========================================
  Files           7        9       +2     
  Lines         385      654     +269     
==========================================
+ Hits          356      619     +263     
- Misses         29       35       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

So, I was initially a bit hesitant to add Test as a dependency. There are a couple of packages which can help with enforcing interfaces (until the day this will be a built-in feature):

but they both internally depend on Test for the same reason 😅 Not sure it's worth looking into those packages right now, since the work for testing the interfaces was already there, and this PR is UP already.

@matt-graham
Copy link
Member Author

So, I was initially a bit hesitant to add Test as a dependency. There are a couple of packages which can help with enforcing interfaces (until the day this will be a built-in feature):

* [`RequiredInterfaces.jl`](https://github.com/Seelengrab/RequiredInterfaces.jl)

* [`Interfaces.jl`](https://github.com/rafaqz/Interfaces.jl)

but they both internally depend on Test for the same reason 😅 Not sure it's worth looking into those packages right now, since the work for testing the interfaces was already there, and this PR is UP already.

Thanks for the pointers, I had wondered if there was existing packages for doing helping do these sort of checks! Agree also that depending on Test in package feels a bit weird but also couldn't see an easy way to avoid while still getting relatively useful feedback on test failures.

I think a pre-requisite of using those packages would be to make our interface more well defined by having some abstract type models are expected to derive from (and also using this to differentiate between general models and those with the conditional-Gaussian structure that allows using optimal proposal), which I think is something we have discussed at various points. I think that would be a worthwhile change to do but probably better as a separate PR. I'll create an issue to remind us to look at this and also the above packages at some point!

@matt-graham matt-graham merged commit 238af83 into main Dec 18, 2024
13 checks passed
@matt-graham matt-graham deleted the mmg/test-functions-in-package branch December 18, 2024 12:44
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.

2 participants