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

hamilton_filter: Clarify the API #634

Merged
merged 1 commit into from
Nov 22, 2022
Merged

Conversation

rht
Copy link
Contributor

@rht rht commented Jun 8, 2022

Aside, the docstring also has contradictory content:

[p] Must be greater than h.

and

e.g. For quarterly data, h = 8 and p = 4 are recommended.

As such, I haven't added a check to ensure that p > h.

@pep8speaks
Copy link

pep8speaks commented Jun 8, 2022

Hello @rht! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-06-08 04:21:06 UTC

@coveralls
Copy link

coveralls commented Jun 8, 2022

Coverage Status

Coverage decreased (-0.001%) to 95.047% when pulling 1d296fc on rht:hamilton_filter into 393c8c3 on QuantEcon:master.

@mmcky
Copy link
Contributor

mmcky commented Jun 8, 2022

@Shunsuke-Hori would you have time to review this?

thanks @rht for the PR.

@Shunsuke-Hori
Copy link
Contributor

Aside, the docstring also has contradictory content:

[p] Must be greater than h.

and

e.g. For quarterly data, h = 8 and p = 4 are recommended.

As such, I haven't added a check to ensure that p > h.

I don't quite remember why I wrote so... Perhaps I misunderstood h with another parameter when I initially wrote the code and forgot to delete the requirement when I push it here. In any case, p > h is not required.

@Shunsuke-Hori
Copy link
Contributor

I created another PR #637 for the fix of the doc

@Shunsuke-Hori
Copy link
Contributor

I made a very small comment, but other than that, it looks good to me.

@mmcky
Copy link
Contributor

mmcky commented Jun 26, 2022

thanks @Shunsuke-Hori and @rht

@mmcky mmcky merged commit a3f38a9 into QuantEcon:master Nov 22, 2022
@rht rht deleted the hamilton_filter branch November 22, 2022 05:32
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