Skip to content

Conversation

jessegrabowski
Copy link
Member

VARMAX forecasting is broken when there is no exogenous data because the return value of data_info is incorrect. I also added tests that would have caught this bug.

Copy link
Contributor

@Dekermanjian Dekermanjian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jesse, the tests looked good to me!

mode=mode,
)

self._needs_exog_data = exog_state_names is not None and len(exog_state_names) > 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is len(exog_state_names)>0 intended to handle when a user inputs an empty list? What about if a user inputs a dictionary with an empty list? I think if you have something like {'endog1': [], 'endog2': []} then you will get a True.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, I switched it to be a bit more robust to all possible cases.

Copy link
Contributor

@Dekermanjian Dekermanjian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think while this became a little bit wordy, it will help the user navigate input errors. I do wonder, though, since other state space models and if I'm not mistaken the structural api use a similar input schema if it would be a good idea to wrap that up in like a _validate_endog_exog_inputs() utility?

@jessegrabowski
Copy link
Member Author

I do wonder, though, since other state space models and if I'm not mistaken the structural api use a similar input schema if it would be a good idea to wrap that up in like a _validate_endog_exog_inputs() utility?

Yes I agree the validation functionality should be moved out to a helper. Can you open an issue? :)

@Dekermanjian
Copy link
Contributor

I do wonder, though, since other state space models and if I'm not mistaken the structural api use a similar input schema if it would be a good idea to wrap that up in like a _validate_endog_exog_inputs() utility?

Yes I agree the validation functionality should be moved out to a helper. Can you open an issue? :)

Yes, I'd be happy to!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working statespace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants