Add Accumulated State Densities Filter #734
Conversation
Squashed and rebased commit based on #309
Also neatened up the code in a few places.
sdhiscocks
requested review from
nperree-dstl and
hpritchett-dstl
and removed request for
a team
Codecov ReportBase: 94.79% // Head: 94.90% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
==========================================
+ Coverage 94.79% 94.90% +0.10%
==========================================
Files 168 170 +2
Lines 8190 8413 +223
Branches 1560 1234 -326
==========================================
+ Hits 7764 7984 +220
- Misses 317 318 +1
- Partials 109 111 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
By using the original Kalman methods, subscripting the ASD Gaussian State, this should also more easily enable the other Kalman variants. This also removes the original transition function method which was unused.
|
Code coverage highlighted that |
stonesoup/predictor/asd.py
Outdated
| covars = \ | ||
| [prior.multi_covar[i * ndim:(i+1) * ndim, i * ndim:(i+1) * ndim] | ||
| for i in range(t_index)] | ||
| covars.append(p_pred_m) |
There was a problem hiding this comment.
I think this should be p_pred_m_cr rather than p_pred_m. Changing this, it seems to give better and more stable results, whereas without changing it, the estimates look wrong.
There was a problem hiding this comment.
I've changed this in bc3e360, which also includes changing the covariances stored in correlation matrices, as I believe this were the wrong way around.
There was a problem hiding this comment.
Sorry for the late answer. You are right. This was an error in my implementation.
Based on equation 20 from 1st reference, verifying based on equations 69-75 from 2nd reference, it seems the predicted covariance given m-1 and given k were switched in there usage after calculation.
sdhiscocks
force-pushed
the
accum_state_dens
branch
from
ecfd1a6 to
bc3e360
Compare
|
@jogehl Thanks again for your contribution, and sorry about significant delay getting to your original pull request. I wasn't sure if you were still active on GitHub. I wanted to add you to the copyright authors list, but wasn't sure whether to put your name or company/institution that you are/were based at when you did this work? |
|
@sdhiscocks if you like you can put the Fraunhofer FKIE to the contributors' list. |
Given other elements of the state are all accessed by index, it is preferable to also have correlation matrices to be the same, to ensure that entire state stays aligned. In particular in edge case where two estimates may be present at the same timestamp.
Co-authored-by: Nicola Perree <[email protected]>
This is an update of #309 by @jogehl to latest version of Stone Soup.
Changes to core code are minor tidy up for readability, some minor changes based on comments from #309, and moving to separate sub-module from Kalman.
Tutorial has also been updated to take advantage of newer animation and plotly features.