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

Fixing Sarima with exogenous variables #75

Closed
wants to merge 16 commits into from
Closed

Fixing Sarima with exogenous variables #75

wants to merge 16 commits into from

Conversation

Axemen
Copy link
Contributor

@Axemen Axemen commented Jul 2, 2021

When using SARIMA it would not pass exogenous variables to the underlying statsmodels.tsa.statespace.sarimax.SARIMAXResults function.

Added a check to ensure the user provides the exogenous variable needed for forecasting if they provided an exogenous variable in the SARIMAParams initialization.

Added test cast for exogenous variable with a daily frequency

I'm not exactly sure how to update the documentation since this added a new parameter to the kats.models.SARIMA.predict function and would like further guidance on how to do so.

Kats

Thank you for contributing! Please take a moment to review our contributing guidelines
to make the process easy and effective for everyone involved.

  • Fork the repo and create your branch from master.
  • If you've added code that should be tested, add tests.
  • If you've changed APIs, update the documentation.
  • Ensure the test suite passes.
  • Make sure your code lints.
  • If you haven't already, complete the Contributor License Agreement ("CLA").

Contributor License Agreement ("CLA")
In order to accept your pull request, we need you to submit a CLA. You only need to do this once to work on any of Facebook's open source projects.

Complete your CLA here: https://code.facebook.com/cla

Ver: Kats v1.02

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 2, 2021
@kirkdotcam
Copy link

kirkdotcam commented Jul 2, 2021

We also weren't sure if we should allow exog to be passed as a named parameter or a kwarg, so here is the code for the kwarg version if that's preferred:

if (self.params.exog is not None) and (kwargs.get('exog') is None):
    msg = "SARIMA model was initialized with exogenous variables. Exogenous variables must be used to predict. use `exog=`"
    logging.error(msg)
    raise ValueError(msg)
fcst = self.model.get_forecast(steps, exog=kwargs.get('exog'))

@facebook-github-bot
Copy link
Contributor

@iamxiaodong has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@jeffhandl merged this pull request in 90b69ed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants