Add nonlinear constant turn models. Update transition model class hierarchy #506
Conversation
DaveKirkland
requested review from
sdhiscocks and
idorrington-dstl
and removed request for
a team
| # class CombinedLinearGaussianTransitionModel(LinearModel, _CombinedGaussianTransitionModel): | ||
| # r"""Combine multiple models into a single model by stacking them. | ||
|
|
||
| The assumption is that all models are Linear and Gaussian. | ||
| Time Variant, and Time Invariant models can be combined together. | ||
| If any of the models are time variant the keyword argument "time_interval" | ||
| must be supplied to all methods | ||
| """ | ||
| # The assumption is that all models are Linear and Gaussian. | ||
| # Time Variant, and Time Invariant models can be combined together. | ||
| # If any of the models are time variant the keyword argument "time_interval" | ||
| # must be supplied to all methods | ||
| # """ | ||
|
|
||
| def matrix(self, **kwargs): | ||
| """Model matrix :math:`F` | ||
| # def matrix(self, **kwargs): | ||
| # """Model matrix :math:`F` | ||
|
|
||
| Returns | ||
| ------- | ||
| : :class:`numpy.ndarray` of shape\ | ||
| (:py:attr:`~ndim_state`, :py:attr:`~ndim_state`) | ||
| """ | ||
| # Returns | ||
| # ------- | ||
| # : :class:`numpy.ndarray` of shape\ | ||
| # (:py:attr:`~ndim_state`, :py:attr:`~ndim_state`) | ||
| # """ | ||
|
|
||
| transition_matrices = [ | ||
| model.matrix(**kwargs) for model in self.model_list] | ||
| return block_diag(*transition_matrices) | ||
| # transition_matrices = [ | ||
| # model.matrix(**kwargs) for model in self.model_list] | ||
| # return block_diag(*transition_matrices) |
There was a problem hiding this comment.
If the entire class is commented-out, it should probably be deleted. Though I think having the 'linear' combined model is still useful. Applying the combined transition via a single matrix multiplication will be quicker than the model-by-model logic of the combined parent class.
There was a problem hiding this comment.
Yes, it looks like I forgot to remove that.
To handle time varying models, the total matrix has to be rebuilt at each call. Also because the total matrix is block diagonal, the full matrix multiply involves a lot of multiply by zero operations. It may make sense to do the smaller matrix multiplies and stitch together the result. It would probably be useful to benchmark the results to see if there is a significant performance hit.
There was a problem hiding this comment.
@oharrald-Dstl @sdhiscocks I've done some benchmarking and indeed the CombinedLinearGaussianTransitionModel is faster than the general implementation. I will add it back in, but utilize the new class hierarchy.
There was a problem hiding this comment.
I have updated the branch. I added back the CombinedLinearGaussianTransitionModel - this will also avoid breaking much of the existing code. I also fixed some of the issues may changes broke surrounding the simple initiator and it's associated test scripts. These issues were cause by code like isinstance(xxx, NonLinear).
DaveKirkland
force-pushed
the
Nonlinear_Models
branch
3 times, most recently
from
155cd96 to
8974993
Compare
DaveKirkland
force-pushed
the
Nonlinear_Models
branch
from
8974993 to
3a18a3c
Compare
| @@ -1,28 +1,17 @@ | |||
|
|
|||
There was a problem hiding this comment.
Do you want to introduce edits to this file?
| @@ -15,7 +15,7 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
Do you want to introduce edits to this file?
|
As there are now two |
Good point. I wonder whether it would better to change name of the models? What about |
@oharrald-Dstl @sdhiscocks |
|
@sdhiscocks I refreshed the commit with the new class names and merged with the main branch. I'm not sure why it has changes for the Video_Processing.ipynb and associated rst and gif files - I didn't changed them. I also tried rebasing with HEAD~2, but this picks up more than 20 previous commits and I wasn't quite sure how to handle that. |
sdhiscocks
added
the
breaking
sdhiscocks
force-pushed
the
Nonlinear_Models
branch
from
9140e92 to
c8077bb
Compare
Codecov Report
@@ Coverage Diff @@
## main #506 +/- ##
==========================================
+ Coverage 94.32% 94.36% +0.03%
==========================================
Files 154 154
Lines 7352 7435 +83
Branches 1399 1410 +11
==========================================
+ Hits 6935 7016 +81
- Misses 314 315 +1
- Partials 103 104 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
I reverted the changes to the video processing demo, and then squash it back into your original commit, rebased onto main. Also added a extra commit, just to fix documentation failure. |
Added nonlinear constant turn models. Moved nonlinear class up into the model i.e. all models are inherently nonlinear. Linear models are a subclass. Added tests for the StateVectors class.
Replaced CombinedLinearGaussianTransitionModel and CombinedNonlinearGaussianTransitionModel with CombinedGaussianTransitionModel which is now in the base class. This will break a lot of existing code that used these combined models but it is an easy fix e.g. replace CombinedLinearGaussianTransitionModel with CombinedGaussianTransitionModel.