Skip to content
This repository has been archived by the owner on Jul 14, 2024. It is now read-only.

add unit tests for Kalman filter library #41

Open
murphyk opened this issue Apr 22, 2022 · 13 comments
Open

add unit tests for Kalman filter library #41

murphyk opened this issue Apr 22, 2022 · 13 comments
Assignees
Labels
enhancement New feature or request

Comments

@murphyk
Copy link
Member

murphyk commented Apr 22, 2022

Verify that the JSL kalman filter / smoother code returns the same marginal means, covariances and log marginal likelihood as when using tfd.LinearGaussianModel,
similar to the 1d test used in kalman_sampler_demo.ipynb.

@gerdm gerdm added the enhancement New feature or request label Apr 22, 2022
@maheswarantp
Copy link

maheswarantp commented May 1, 2022

I would like to work on this one if its okay. Sorry for the late response, I had exams scheduled during this time

@murphyk
Copy link
Member Author

murphyk commented May 1, 2022 via email

@gerdm
Copy link
Collaborator

gerdm commented May 5, 2022

Hi @maheswarantp,

should I assign this issue to you?

@maheswarantp
Copy link

Sure, I am working on this

@gerdm gerdm changed the title add unit tests for kalman library add unit tests for Kalman filter library May 7, 2022
@maheswarantp
Copy link

maheswarantp commented May 12, 2022

Minor changes: @murphyk I am just wrapping this, you can see my code here will send a PR in a day or two.

TODO

  • Usage of different covariance comparison techniques other than Sum of Squared Distances
  • Comparisons of Log Likelihoods
    It would be a great help if you could suggest any changes if needed

@gerdm
Copy link
Collaborator

gerdm commented May 12, 2022

@maheswarantp, can you also create a script for this? We want to add automatic unit-tests for future releases of JSL.

@murphyk
Copy link
Member Author

murphyk commented May 12, 2022

Unit tests should be scripts which have statements like assert np.allclose(A,B,tol=1e-2) that can be checked automatically, without needing to inspect plots by eye.

You define the model twice, once using TFP notation, and once using JAX. This introduces the possibility of inconsistency.
You should define the parmaeters once (eg as numpy arrays) and then convert to TFP or JSL objects.

Screen Shot 2022-05-12 at 10 52 38 AM

Most importantly, from your plot it looks like JSL and TFP ave slighlty different posteriors for time step 1,
presumably due to the edge case of how the prior and/ or initial observation is used. Please resolve this discrepancy.

Screen Shot 2022-05-12 at 10 52 24 AM

@murphyk
Copy link
Member Author

murphyk commented May 12, 2022

@maheswarantp
Copy link

Okay, I will work on that, thanks :)

@maheswarantp
Copy link

Building on @gileshd kalman_filter_test.py, I have added a few more unit tests which could be useful. They are

  • Test to ensure proper behavior of the kalman filter as comapred to TFP when random noise in measurements is induced
  • Test to ensure proper behavior of the kalman filter as compared to TFP when initial prior measurements are randomized

I have also extended the unit tests to include the kalman smoother code, testing the posterior sampling of JSL and TFP here

Also @murphyk , regarding the unit tests for log marginal likelihoods asked, do you mean the conditional likelihood computed in kalman smoother code or something different?

@murphyk
Copy link
Member Author

murphyk commented May 17, 2022

@maheswarantp Please make a proper PR. Also please use pytest framework.

@gileshd
Copy link
Collaborator

gileshd commented May 17, 2022

@maheswarantp no need for playing around with paths with:
sys.path.insert(0,os.path.abspath(os.path.dirname(__file__)))
(e.g. here).

You can just use absolute imports as is done in #54 (and discussed in issue #53).

@gileshd
Copy link
Collaborator

gileshd commented May 17, 2022

@maheswarantp re kalman_filter_initial_prior_test(), I imagine having unseeded random numbers (i.e. the calls to np.random) in a test might make it quite hard to debug because it will be hard to reproduce the failure case.

In fact this test should probably fail because the current implementation of tfp_filter does not include an argument to change the prior covariance, it is set as the identity matrix:

    prior = tfd.MultivariateNormalDiag(mu0, tf.ones([state_size]))

Therefore the random numbers in kalman_filter_initial_prior_test only affect the JSL kalman filter not the tfp one.

I think in general this particular test probably isn't necessary or suitable for as part of a suite of automated tests.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants