Granger cleanup and nobs check #1018
Conversation
|
From a quick read-through the caching of
What error did you get here? My "impression": multicollinearity (even perfect) within a time series is irrelevant for the hypothesis test, because we are not testing individual coefficients, but a joint hypothesis for a group of coefficients. |
|
@josef-pkt You are correct. I kept the caching in the OLS results because it clears a few TODOs in the code, but I removed a check I had placed in the granger section. Happy to split this into to PRs if you prefer. |
|
@josef-pkt This should be ready to go. |
| @@ -464,12 +462,12 @@ def pacf_yw(x, nlags=40, method='unbiased'): | |||
| This solves yule_walker for each desired lag and contains | |||
| currently duplicate calculations. | |||
| ''' | |||
| xm = x - x.mean() | |||
There was a problem hiding this comment.
this looks like a bug not cleanup #1039
There was a problem hiding this comment.
xm is never used below. Maybe a typo / copy-paste-o.
There was a problem hiding this comment.
yes, I'm pretty sure that the call to yule_walker should use xm and not x.
(But I just did a quick check with pdb in nosetests and didn't look at all the details yet. It doesn't look like x is demeaned before calling pacf_yw.)
There was a problem hiding this comment.
False alarm here, I closed #1039 again, we have unittests and yule_walker demeans by default.
removing this line is good, and it won't get me confused next time
|
This is now rebased on master. |
|
|
||
| # create lagmat of both time series | ||
| dta = lagmat2ds(x, mxlg, trim='both', dropex=1) | ||
|
|
||
| #add constant | ||
| if addconst: | ||
| dtaown = add_constant(dta[:,1:mxlg+1], prepend=False) | ||
| dtajoint = add_constant(dta[:,1:], prepend=False) | ||
| dtaown = add_constant(dta[:, 1:mxlg+1], prepend=False) |
There was a problem hiding this comment.
the rule now also says spaces around operators in slices
I think it's [:, 1 : mxlg + 1] what we started to use. (pep-8 now says not spaces around the innermost operator, but we don't adjust for that, except **)
|
Thanks, looks good and it's a clean rebase. cosmetic comments to cosmetic changes |
|
after the rebase the second commit message "ENH: Added condno and eigenvalue caches to OLS and Quantile regression " is now inappropriate. maybe an interactive rebase to edit the commit message to In general, we try not to make style/pep-8 changes and substantial changes in the same commit. It's difficult to quickly see what the relevant change is in the commit. (for the future) |
|
@guyrt Thank you, merged after interactive rebase There has been unfortunately some delay, given the confusion about what to do with the commits for 0.5.1 In the description you mention checking for collinearity, but I didn't see any code related to that. Was there some in an earlier version? |
Addresses issue #347.
I've included two tests. First is a restriction on the number of observations required for a given max lag.
Second is a test that catches co-linearity between two time series. For now, this actually raises a ValueError. I did this because I was already getting an error and this one is much cleaner. Suggestions welcome.