-
Notifications
You must be signed in to change notification settings - Fork 87
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 the ClarkWestTest for time series #203
Conversation
@PaulSoderlind we must complete the |
OK, expect something early next week. |
I am now (23 May) trying
The reason for asking is that I (of course) get a similar error message when I try to use the skeleton files for the CW test. |
Indeed it had an error, I fixed it in #205 and corrected the |
I now have files for src and test for this (see below for cut and paste). Two questions:
src
test
|
1 - This PR is ready, if you want I can close it and you open a new one with the exact same changes so GitHub understands that you made the contribution. I have no problem with that, it is up to you :) 2 - I agree it should be a |
I am happy to add/change the current PR, but I am no good at this Github machinery. What is the easiest way of doing it? Edit the files here (copy and paste will probably work)? |
you can git clone |
@guilhermebodin I just checked the src and test files on your clark-west branch - and they look fine (thanks for the changes). What else is needed? doc? If so, can you do that? |
I think we are good, I am going to request for some reviews. |
@mschauer I am not able to click in the reviewers button, could you review this PR? |
Thanks guys! |
Co-authored-by: Moritz Schauer <[email protected]>
Co-authored-by: Moritz Schauer <[email protected]>
@mschauer is it good? |
Please let me know if there is anything I can do to make this move forward. |
Hey guys while we're talking about tests for predictive accuracy, have you seen @colintbowers' ForecastEval.jl?
|
Hi all. Yes please feel free to integrate anything from ForecastEval.jl that is of use. I'm happy to help as much as I'm able. One point probably worth noting also is that if you check out ForecastEval.jl, you'll note it depends on DependentBootstrap.jl, which is another registered package of mine. DependentBootstrap.jl implements the iid, stationary, circular, and moving block bootstraps, along with associated block-length selection procedures and bandwidth selection routines. This might be useful if you ever want to offer variants of some of the hypothesis tests here that use bootstrapping to estimate statistical distributions, such as I have done for my implementation of Diebold-Mariano in ForecastEval.jl. And obviously for some stats like the SPA test and Model Confidence Set, they can only be implemented in practice using a bootstrap. |
Co-authored-by: Moritz Schauer <[email protected]>
@nalimilan Want to have a look before I merge? |
src/clark_west.jl
Outdated
xbar = mean(d) | ||
stderr = sqrt(cw_var) | ||
statistic_cw = xbar/stderr | ||
return ClarkWestTest(n, xbar, stderr, statistic_cw, 0.0) |
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.
Int
is better as a neutral zero as it doesn't force promotion of e.g. Float32
:
return ClarkWestTest(n, xbar, stderr, statistic_cw, 0.0) | |
return ClarkWestTest(n, xbar, stderr, statistic_cw, 0) |
But is it really useful to have this field? Is it required by the ZTest
interface (if it exists at all)?
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.
only used in population_param_of_interest
. Probably not needed
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.
It is on the ZTest interface
struct OneSampleZTest <: ZTest
n::Int # number of observations
xbar::Real # estimated mean
stderr::Real # population standard error
z::Real # z-statistic
μ0::Real # mean under h_0
end
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.
This isn't an interface, it's the definition of a concrete struct inheriting from ZTest
.
|
||
function show_params(io::IO, x::ClarkWestTest, ident) | ||
println(io, ident, "number of observations: $(x.n)") | ||
println(io, ident, "CW statistic: $(x.z)") |
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.
IIUC this is a Z statistic?
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.
yes
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.
I mean, should it be mentioned as such? Also related: is there a point in inheriting from ZTest
?
Thank you! |
@guilhermebodin would you like to go ahead and make the suggested changes? In case you don't have time to do it soon, I can give it a go. If so, would I just fork your repo to get the current files and send a PR to you? Is that how it works? |
@mschauer Can we merge it? |
Comments, @nalimilan? |
This closes #202.