Always return dictionary from data_info#582
Conversation
jessegrabowski
requested review from
AlexAndorra,
Dekermanjian and
ricardoV94
jessegrabowski
force-pushed
the
data-info-default
branch
from
90c92cd to
de748b6
Compare
jessegrabowski
force-pushed
the
data-info-default
branch
from
de748b6 to
445d884
Compare
Dekermanjian
left a comment
Dekermanjian
left a comment
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fair enough, I switched it to be a bit more robust to all possible cases.
Dekermanjian
left a comment
Dekermanjian
left a comment
There was a problem hiding this comment.
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?
Yes I agree the validation functionality should be moved out to a helper. Can you open an issue? :) |
Yes, I'd be happy to! |
* Always return dictionary from data_info * Remove unused docstring parameter * More robust `needs_exog_data` * fix typo
2 participants
VARMAX forecasting is broken when there is no exogenous data because the return value of
data_infois incorrect. I also added tests that would have caught this bug.