dstl / Stone-Soup

A software project to provide the target tracking community with a framework for the development and testing of tracking algorithms.
https://stonesoup.rtfd.io
MIT License
384 stars 126 forks source link

Fix incorrect transition model being used in Kalman Smoothers #945

Closed sdhiscocks closed 4 months ago

sdhiscocks commented 5 months ago

Fixes #941

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (0a9ebe2) 93.24% compared to head (2b3d3f1) 93.24%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #945 +/- ## ======================================= Coverage 93.24% 93.24% ======================================= Files 201 201 Lines 12617 12622 +5 Branches 2589 2591 +2 ======================================= + Hits 11765 11770 +5 + Misses 602 601 -1 - Partials 250 251 +1 ``` | [Flag](https://app.codecov.io/gh/dstl/Stone-Soup/pull/945/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dstl) | Coverage Δ | | |---|---|---| | [integration](https://app.codecov.io/gh/dstl/Stone-Soup/pull/945/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dstl) | `66.59% <30.76%> (-0.04%)` | :arrow_down: | | [unittests](https://app.codecov.io/gh/dstl/Stone-Soup/pull/945/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dstl) | `88.64% <100.00%> (+<0.01%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dstl#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

sdhiscocks commented 4 months ago

@narykov Does this resolve #941?

narykov commented 4 months ago

@narykov Does this resolve #941?

Apologies I didn't get back to this earlier. I can confirm this has resolved the issue. Thank you!

narykov commented 4 months ago

@narykov Does this resolve #941?

Apologies I didn't get back to this earlier. I can confirm this has resolved the issue. Thank you!

Perhaps, the only additional comment I have is that in my case each prediction in the track is accompanied by its own transition_model, so I find it puzzling that I am forced to specify a single transition model when calling the smoother. As a consequence, I had to specify some dummy model that the smoother ends up never using.

sdhiscocks commented 4 months ago

Apologies I didn't get back to this earlier. I can confirm this has resolved the issue. Thank you!

Thanks for confirming.

Perhaps, the only additional comment I have is that in my case each prediction in the track is accompanied by its own transition_model, so I find it puzzling that I am forced to specify a single transition model when calling the smoother. As a consequence, I had to specify some dummy model that the smoother ends up never using.

So hopefully setting it to None should work okay? But maybe we should make it default None so it's optional?

narykov commented 4 months ago

So hopefully setting it to None should work okay? But maybe we should make it default None so it's optional?

Setting transition_model=None works for me, just it hasn't crossed my mind that there is that possibility. And as for the second question, I am thinking that it is reasonable to have transition_model explicitly specified as optional because:

Thanks!

narykov commented 4 months ago

The supplied transition_model is primarily used as a fallback option, which won't be used unless the track updates lack their own models. Furthermore, one might mistakenly believe that it is the supplied model that is employed for processing, whereas another model is actually implemented inside the smoother.

Perhaps, if transition_model is specified explicitly in the definition of smoother it should be enforced by the smoother? As this appears to me as the expected behaviour.