Skip to content

Conversation

@jstac
Copy link
Contributor

@jstac jstac commented Apr 7, 2015

@spencerlyon2 @cc7768 @mmcky

Hi all, there was a discussion of whether or not Kalman should subclass the linear state space class (formerly LSS, now LinearStateSpace) from lss.py, or vice versa. See #115.

In retrospect I don't think subclassing is the right way to go. For example, a Kalman filter isn't a special kind of linear state space model. A better way to think of it is that a linear state space system is part of the data needed to set up a Kalman filter. The other part of the data is the prior distribution. Hence I've rewritten Kalman to take an instance of LinearStateSpace as one of its arguments. This replaces the individual matrices from the transition and observation equations.

Basically we are replacing inheritance with composition, which seems a better fit in this case.

Among the advantages of the new version of Kalman compared to the previous one are that we don't need the logic for checking the matrices, coercing them into arrays of the right shape, etc., in two places. Now it's just in lss.py. Second, once we've created an instance of LinearStateSpace we have access to its functionality if required.

All thoughts are appreciated.

@davidrpugh
Copy link
Contributor

Seems good refactor to me. Good case to favor composition over
inheritance...

On Tue, Apr 7, 2015 at 12:38 PM, John Stachurski notifications@github.com
wrote:

@spencerlyon2 https://github.com/spencerlyon2 @cc7768
https://github.com/cc7768 @mmcky https://github.com/mmcky

Hi all, there was a discussion of whether or not Kalman should subclass
the linear state space class (formerly LSS, now LinearStateSpace) from
lss.py, or vice versa. See #115
#115.

In retrospect I don't think subclassing is the right way to go. For
example, a Kalman filter isn't a special kind of linear state space model.
A better way to think of it is that a linear state space system is part of
the data needed to set up a Kalman filter. The other part of the data is
the prior distribution. Hence I've rewritten Kalman to take an instance of
LinearStateSpace as one of its arguments. This replaces the individual
matrices from the transition and observation equations.

Basically we are replacing inheritance with composition, which seems a
better fit in this case.

Among the advantages of the new version of Kalman compared to the previous
one are that we don't need the logic for checking the matrices, coercing
them into arrays of the right shape, etc., in two places. Now it's just in
lss.py. Second, once we've created an instance of LinearStateSpace we have
access to its functionality if required.

All thoughts are appreciated.

You can view, comment on, or merge this pull request online at:

#134
Commit Summary

  • proposed new version of Kalman class
  • modified the kalman test

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#134.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 90.43% when pulling cc4704b on update_kalman into 5316070 on master.

@cc7768
Copy link
Member

cc7768 commented Apr 7, 2015

This looks clean. Good changes.

@jstac
Copy link
Contributor Author

jstac commented Apr 14, 2015

Tom has signed off, solutions updated, just need to update lecture correspondingly.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 90.43% when pulling 59a5094 on update_kalman into 5316070 on master.

@albop
Copy link
Contributor

albop commented Apr 15, 2015

The refactored version looks good to me.
I just bumped into this piece of news: https://www.google-melange.com/gsoc/project/details/google/gsoc2014/reo/5808496591241216 . One should keep an eye on this project. It could be very useful.

@jstac jstac merged commit 59a5094 into master Apr 17, 2015
@sglyon sglyon deleted the update_kalman branch December 10, 2015 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants