Kalman filter with inputs #5
Conversation
| @@ -166,7 +191,7 @@ Kalman filter | |||
| function kalmanfilter(yy::AbstractMatrix, M::LinearHomogSystem) | |||
| d = size(M.Phi, 1) | |||
| n = size(yy, 2) | |||
| kalmanfilter!(0:n, yy, zeros(d, n), zeros(d, d, n), zeros(d, d, n), M) | |||
| kalmanfilter!(1:n, yy, zeros(d, n), zeros(d, d, n), zeros(d, d, n), M) | |||
There was a problem hiding this comment.
Ups, this is a mistake while rebasing.
Codecov Report
@@ Coverage Diff @@
## master #5 +/- ##
==========================================
+ Coverage 68.59% 71.27% +2.67%
==========================================
Files 6 7 +1
Lines 242 275 +33
==========================================
+ Hits 166 196 +30
- Misses 76 79 +3
Continue to review full report at Codecov.
|
|
I won't have time to look into it until next week, unfortunately, but so far it looks very nice. I assume that we'll need a function like EDIT: Nevermind, that's your |
|
It works! I get the same results as with my PR.
If I'm understanding things correctly, you do things this way because it's awkward to talk about the transition from t=0 to t=1, so you'd rather let the user directly specify the post-transition probability? The output I'm fine with merging this instead of my PR. |
| if s == t # if `s == t`` no time has passed | ||
| x + u, P, one(M.Phi) | ||
| else | ||
| M.Phi*x + M.b + u, M.Phi*P*M.Phi' + M.Q, M.Phi |
There was a problem hiding this comment.
I think I understand why you have both M.b and u, but the redundancy makes me a bit uncomfortable. They're the same quantity. Perhaps we should assert that uu==nothing when using LinearEvolution?
There was a problem hiding this comment.
How I think of them is that b is the intrinsic drift and u is an control/external signal.
There was a problem hiding this comment.
Perhaps, but unless I'm mistaken, u is unspecified (implicitly 0?) when GenericLinearEvolution is used.
There was a problem hiding this comment.
GenericLinearEvolution will dispatch on function https://github.com/mschauer/Kalman.jl/pull/5/files/0826886fe9698002c68cc8c16c3c8c39a6e8d92e#diff-d0bf3c72d95233c17cc7933aeaa30298R37
There was a problem hiding this comment.
Right. What I mean is that with LinearObservation (line 33), the evolution is Phi*x + M.b + u (transition matrix + intrinsic drift + control/external signal), whereas with GenericLinearEvolution (line 39), it's Phi*x + b
Anyway, it's no big deal; just seemed a bit inconsistent.
|
|
||
|
|
||
| function predict!(s, x, P, t, u, M) | ||
| if s == t # if `s == t`` no time has passed |
There was a problem hiding this comment.
Shouldn't this special-casing also be done in predict!(s, x, P, t, U, M::GenericLinearEvolution)?
|
Perhaps timesteps should be assumed homogeneous, unless using |
|
In https://github.com/mschauer/Kalman.jl/pull/5/files#diff-d0bf3c72d95233c17cc7933aeaa30298R38 |
|
|
||
| struct GenericLinearObservation <: AbstractObservation | ||
| end | ||
| struct GenericLinearEvolution{TPhi,Tb,TQ} <: AbstractEvolution |
There was a problem hiding this comment.
Is there a need for this type to be parametric? GenericLinearObservation is parameter-less.
|
I started building code on top of this PR. Is there any significant reason not to merge it? Small issues can be fixed afterwards... |
|
You are right, das Bessere ist der Feind des Guten ("The best is the enemy of the good.") |
|
Doch! |
@cstjean This would be my design for #4 . Can you give it a try? See the test file
testinput.jlhow to use the new set-up, a varyingbwould be achieved with theGenericLinearObserver.Note that the time runs from
1:nin the new parts and the prior is assumed to be valid for time1.Smoother and all convenience wrapper as well as iterators are still missing.