-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add sparse kalman filter #290
Conversation
Codecov Report
@@ Coverage Diff @@
## master #290 +/- ##
==========================================
- Coverage 93.88% 92.66% -1.23%
==========================================
Files 33 34 +1
Lines 3043 3164 +121
==========================================
+ Hits 2857 2932 +75
- Misses 186 232 +46
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's really nice!
Looking at this PR, the main thing that comes to my mind is there seems to be a lot of duplicate code from the non-sparse types. Is it really necessary to have a new type specific for the sparse state? First, it looks that the types in SparseUnivariateKalmanState
aren't sparse, just some calculations that happen in the filter, and second, if we want to allow SparseVector
and SparseMatrixCSC
, we could relax the types of the non-sparse state types to AbstractVector
and AbstractMatrix
.
But these are just my thoughts after taking a quick look, it might not be so simple.
@raphaelsaavedra agreed, I am going to try to relax the type on the StateSpaceSystems and KalmanStates. My main concern was that not all AbstractArrays work with
|
Well I gave it a try and the implementation to consider every vector ended up in much more allocations and bigger compile times
for comparison, this is the one in master (and the one with the sparse filter on a separated file)
I think we are going with the sparse filter in a separated file with a lot of duplicated code for now |
@guilhermebodin, @AndreFCR tried to run our model (the one with trend, slope and two stochastic seasonalities with s = 24 and s = 168) and the running time with the sparse kalman filter was considerably higher. We will try to investigate more why this is happening and return to you. |
@marinadietze the speed ups were identified on julia 1.6 |
@raphaelsaavedra, There is a project going on where @AndreFCR and @marinadietze have a system with ~200 states. It currently takes about 50 minutes to estimate the system for 25k observations with some relaxation on the gradient norm. Let's see how low they can go.