[ENH] Add full exogenous variable support for statsmodels adapter _update() method when update_params=False #8626
Conversation
Update exog in update for statsmodels adapter
There was a problem hiding this comment.
Thanks a lot!
We should at least add a test for the bug you encountered.
What worries me more is that the test suite in TestAllForecasters did not catch this - optional but it would be really great if you could also add the test there, with an abstract forecaster!
|
Thank you for your review ! I have added tests to make sure this bug is correctly catched. Let me know if I missed something |
|
nice! I have started the tests now. |
There was a problem hiding this comment.
Code formatting is failing, see here how to ensure automatic formatting: https://www.sktime.net/en/stable/developer_guide/coding_standards.html
|
Alright, should be good now. |
|
Interesting - it looks like some other models are failing the new test - they should obviously pass. This is how I would suggest how to deal with this:
When that is done, we can merge this (so we avoid adding unskipped failures to |
|
(let us know if you would like someone else to do that) |
The StatsForecastAutoARIMA forecaster seemed to fail the test because the exog matrix I used in the test contained integer instead of floating values. I'm not sure whether this is intended behaviour or not but I updated the test accordingly so that this should pass. Let me know If you would rather have the test succeeding even if the matrix is composed of integer values |
Reference Issues/PRs
Fixes #8096
What does this implement/fix? Explain your changes.
These changes allow the use of exogenous variables when using the _update() method from a statsmodels adapter with update_params=False (encountered this issue when applying grid search with strategy='no-update_params' to a SarimaX estimator)
Does your contribution introduce a new dependency? If yes, which one?
No
What should a reviewer concentrate their feedback on?
Only 2 lines of code are added, so I think the review would be straightforward
Did you add any tests for the change?
No
Any other comments?
My first ever pull request on a public repo, so be kind haha