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

BUG/ENH: VARMAX Fix trend / exog. timing. Add polynomial trends. #4766

Merged
merged 6 commits into from
Jun 29, 2018

Conversation

ChadFulton
Copy link
Member

VARMAX models with trend or exog used a misleading timing convention that I think qualifies as type-bug-wrong. The timing convention it used was as according to the general state space transition equation:

\alpha_{t+1} = d_t + T_t \alpha_t + \eta_t

But for a VAR, \alpha_t = y_t, and so trend / exog for time t would be associated with the endog for t+1.

This fixes this problem and also adds a more extensive set of trends (allows a polynomial trend like in SARIMAX rather than just trend='c').

Also adds a bunch of unit tests for VARX models with and without trends against the R package "vars".

@ChadFulton ChadFulton added comp-tsa type-bug type-bug-wrong serious bugs that silently return incorrect numbers labels Jun 29, 2018
@josef-pkt
Copy link
Member

This pull request introduces 1 alert and fixes 1 when merging 7501356 into 450f30d - view on LGTM.com

new alerts:

  • 1 for Unused local variable

fixed alerts:

  • 1 for Unused import

Comment posted by LGTM.com

@coveralls
Copy link

coveralls commented Jun 29, 2018

Coverage Status

Coverage increased (+0.004%) to 83.646% when pulling d6d1b3d on ChadFulton:varmax-exog into 450f30d on statsmodels:master.

@josef-pkt
Copy link
Member

This pull request introduces 1 alert and fixes 1 when merging e16ef78 into 450f30d - view on LGTM.com

new alerts:

  • 1 for Unused local variable

fixed alerts:

  • 1 for Unused import

Comment posted by LGTM.com

@ChadFulton
Copy link
Member Author

ChadFulton commented Jun 29, 2018

This PR also switches from raising an error on non-stationary or non-invertible starting params to giving a warning. This is the same as what #4739 did for SARIMAX.

Edit: this could also be pulled out of this PR since it's not related, if that was cleaner.

@ChadFulton
Copy link
Member Author

This is coming up green. Since it's a bigger PR, I'll wait a bit (tomorrow or on the weekend) before merging (or longer if there are concerns), but it would be good to get the fix in.

@josef-pkt
Copy link
Member

This pull request fixes 1 alert when merging d6d1b3d into 450f30d - view on LGTM.com

fixed alerts:

  • 1 for Unused import

Comment posted by LGTM.com

@codecov-io
Copy link

codecov-io commented Jun 29, 2018

Codecov Report

Merging #4766 into master will increase coverage by <.01%.
The diff coverage is 95.68%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4766      +/-   ##
=========================================
+ Coverage    81.1%   81.1%   +<.01%     
=========================================
  Files         580     581       +1     
  Lines       89615   89796     +181     
  Branches     9991   10028      +37     
=========================================
+ Hits        72678   72833     +155     
- Misses      14657   14683      +26     
  Partials     2280    2280
Impacted Files Coverage Δ
statsmodels/tsa/statespace/tests/test_var.py 100% <100%> (ø)
statsmodels/tsa/statespace/tests/test_varmax.py 99.12% <100%> (-0.01%) ⬇️
statsmodels/tsa/statespace/tests/test_simulate.py 100% <100%> (ø) ⬆️
statsmodels/tsa/statespace/kalman_filter.py 84.16% <50%> (-3.59%) ⬇️
statsmodels/tsa/statespace/tools.py 73.74% <86.2%> (+0.84%) ⬆️
statsmodels/tsa/statespace/varmax.py 91.21% <93.05%> (+0.45%) ⬆️
statsmodels/base/data.py 90.29% <0%> (+0.48%) ⬆️

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 450f30d...d6d1b3d. Read the comment docs.

@ChadFulton ChadFulton merged commit f8004ec into statsmodels:master Jun 29, 2018
@josef-pkt josef-pkt added this to the 0.10 milestone Sep 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-tsa type-bug type-bug-wrong serious bugs that silently return incorrect numbers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants