Skip to content
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 kalman smoother and log likelihood #209

Merged
merged 9 commits into from
Jul 20, 2018

Conversation

Shunsuke-Hori
Copy link
Collaborator

@Shunsuke-Hori Shunsuke-Hori commented Jul 11, 2018

This PR adds Kalman smoother and a function to compute log-likelihood. I also made minor updates to existing code.

Ready for review

PS: I'm wondering if the current kalman.jl is efficient and well documented enough.

@Shunsuke-Hori Shunsuke-Hori changed the title Add kalman smoother Add kalman smoother and log likelihood Jul 11, 2018
@codecov-io
Copy link

codecov-io commented Jul 11, 2018

Codecov Report

Merging #209 into master will increase coverage by 0.16%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   90.76%   90.93%   +0.16%     
==========================================
  Files          25       25              
  Lines        1744     1776      +32     
==========================================
+ Hits         1583     1615      +32     
  Misses        161      161
Impacted Files Coverage Δ
src/kalman.jl 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 785c71b...9caab4c. Read the comment docs.

Copy link
Member

@cc7768 cc7768 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great code and a useful contribution -- Makes it very easy to review when I don't have much to add to what you've done.

Thank you for adding a smoother. We can wait for @sglyon to approve, but I think this is ready to be merged (modulo adding a new line before the ##### Return in docs).

src/kalman.jl Outdated
forecast for period ``t`` observation conditional on ``t-1``
observation.
- `y::AbstractVector`: observations at period ``t``
##### Returns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It improves readability to put a new line to separate the end of the inputs and the beginning of the returns.

There are a few other places this shows up in this file.

B = G * Sigma' * G' + R
M = A * inv(B)
B = G * Sigma * G' + R
M = A / B
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! This is better than taking the inverse.

@Shunsuke-Hori
Copy link
Collaborator Author

@cc7768 updated. Thank you!!

Copy link
Member

@sglyon sglyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is very clean (as usual 👍 ). Thanks for this contribution @Shunsuke-Hori

I haven't had time to check the math, but it looks right at first glance. If one other person could vouch for the validity I think we can merge.

Also, I think your hunch about performance/efficiency is probably correct. This was written more as a textbook implementation and likely leaves some performance on the table.

@Flintro
Copy link

Flintro commented Jul 18, 2018

I double checked the math and equations. Also looks good to me. Really cool contribution all round as well.

@sglyon sglyon merged commit 0320374 into QuantEcon:master Jul 20, 2018
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.

5 participants