JSL icon indicating copy to clipboard operation
JSL copied to clipboard

add unit tests for Kalman filter library

Open murphyk opened this issue 2 years ago • 13 comments

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.

murphyk avatar Apr 22 '22 23:04 murphyk

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

maheswarantp avatar May 01 '22 14:05 maheswarantp

Sure

On Sun, May 1, 2022 at 7:26 AM Maheswaran Parameswaran < @.***> wrote:

I would like to work on this one if its okay :)

— Reply to this email directly, view it on GitHub https://github.com/probml/JSL/issues/41#issuecomment-1114252673, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDK6EGE62VXTK3P6WGFYADVH2IBRANCNFSM5UDW3H6A . You are receiving this because you authored the thread.Message ID: @.***>

-- Sent from Gmail Mobile

murphyk avatar May 01 '22 17:05 murphyk

Hi @maheswarantp,

should I assign this issue to you?

gerdm avatar May 05 '22 17:05 gerdm

Sure, I am working on this

maheswarantp avatar May 06 '22 10:05 maheswarantp

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

maheswarantp avatar May 12 '22 15:05 maheswarantp

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

gerdm avatar May 12 '22 17:05 gerdm

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 avatar May 12 '22 17:05 murphyk

Please use https://docs.python.org/3/library/unittest.html

murphyk avatar May 12 '22 17:05 murphyk

Okay, I will work on that, thanks :)

maheswarantp avatar May 13 '22 04:05 maheswarantp

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

  • [x] Test to ensure proper behavior of the kalman filter as comapred to TFP when random noise in measurements is induced
  • [x] 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?

maheswarantp avatar May 17 '22 19:05 maheswarantp

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

murphyk avatar May 17 '22 20:05 murphyk

@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 avatar May 17 '22 22:05 gileshd

@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.

gileshd avatar May 17 '22 23:05 gileshd