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 Baxter King filter #223

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Shunsuke-Hori
Copy link
Collaborator

test is added too

@Shunsuke-Hori Shunsuke-Hori requested review from sglyon and cc7768 October 8, 2018 16:04
@codecov-io
Copy link

Codecov Report

Merging #223 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   95.87%   95.86%   -0.02%     
==========================================
  Files          25       25              
  Lines        1067     1063       -4     
==========================================
- Hits         1023     1019       -4     
  Misses         44       44
Impacted Files Coverage Δ
src/filter.jl 100% <100%> (ø) ⬆️
src/util.jl 98.14% <0%> (-0.07%) ⬇️
src/markov/markov_approx.jl 98.37% <0%> (-0.04%) ⬇️
src/markov/ddp.jl 98.63% <0%> (-0.02%) ⬇️
src/arma.jl 100% <0%> (ø) ⬆️

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 7fc1254...63ba102. Read the comment docs.

@sglyon
Copy link
Member

sglyon commented Oct 9, 2018

Hi @Shunsuke-Hori thanks for submitting!

I googled for some references on the BK filter equations and found conflicting evidence.

Two references are

A few differences:

  • the html page only divides b[2:end] by (2K+1), but keeps b[1] without dividing
  • The pdf has a negative sign in front of theta, while the html page keeps it positive.

your code seems to more closely follow the pdf, but I'm not sure.

What reference did you use? How do we know which is correct?

@Shunsuke-Hori
Copy link
Collaborator Author

@sglyon Thank you for the review.

I first looked the html but since the output was not same as python's, I switched to the pdf.

I checked the original paper and it seems the pdf is consistent with the paper (although, honestly, I mindlessly trusted the result of `bk_filter' of python when I wrote the code)

@andi3141
Copy link

Hi @Shunsuke-Hori
I had a look at the code. It seems perfectly in line with
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.197.8691&rep=rep1&type=pdf
The second reference
http://help.prognoz.com/en/mergedProjects/Lib/02_time_series_analysis/uimodelling_baxterkingfilter.htm
seems not to be online any more. Further the original reference
https://www.mitpressjournals.org/doi/abs/10.1162/003465399558454?casa_token=pfn7A97wi0QAAAAA:78AlfSOyiz__a6_snyH7jwydLu8uGZV-U3ZVY5Vo3dTi0r7da5uuVtUyUR87-uZBAuarQAYMiywJ76o
is not publicy available (MIT press). I try to get my hands on that at some point.
The implementation gives the same result as the python implementation in
https://www.statsmodels.org/stable/generated/statsmodels.tsa.filters.bk_filter.bkfilter.html#statsmodels.tsa.filters.bk_filter.bkfilter

So I think, it is fine.

src/filter.jl Outdated
- `y::AbstractVector` : data to be filtered
- `wu::Real` : upper cutoff frequencies
- `wl::Real` : lower cutoff frequencies
- `K::Integer` : number of lead and lag of the moving average
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be leads and lags

@jstac
Copy link
Contributor

jstac commented Sep 25, 2020

Many thanks @andi3141 for your review. This looks good to me too (aside from very minor comment above).

@codecov-commenter
Copy link

Codecov Report

Merging #223 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #223      +/-   ##
==========================================
- Coverage   95.87%   95.86%   -0.02%     
==========================================
  Files          25       25              
  Lines        1067     1063       -4     
==========================================
- Hits         1023     1019       -4     
  Misses         44       44              
Impacted Files Coverage Δ
src/filter.jl 100.00% <100.00%> (ø)
src/util.jl 98.14% <0.00%> (-0.07%) ⬇️
src/markov/markov_approx.jl 98.37% <0.00%> (-0.04%) ⬇️
src/markov/ddp.jl 98.63% <0.00%> (-0.02%) ⬇️
src/arma.jl 100.00% <0.00%> (ø)

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 7fc1254...bf39218. Read the comment docs.

@Shunsuke-Hori
Copy link
Collaborator Author

Thank you for reviewing, @andi3141 and @jstac

I fixed the grammar.

@jstac
Copy link
Contributor

jstac commented Sep 26, 2020

Looks good to me. Over to you @sglyon

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