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

Add UnobservedComponents statsmodels wrapper #1394

Merged
merged 34 commits into from
Sep 17, 2021

Conversation

juanitorduz
Copy link
Contributor

@juanitorduz juanitorduz commented Sep 6, 2021

Reference Issues/PRs

Fixes #1371

What does this implement/fix? Explain your changes.

This PR wraps the UnobservedComponents from statsmodels. We follow closely the existing implementation sktime/forecasting/ets.py using the _StatsModelsAdapter class (a minor change was added in order to support exog variables).

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

In this initial PR I provide a basic implementation of the wrapper. Still, I wanna make sure I understand what are the requirements the code needs in order to merge to main. Concretely here are my questions:

Main Questions:

  • The docstrings of the original statsmodels class UnobservedComponents is very complete and gives a clear description of the model and the level variable which determines the structure of the state space model. How much should I add to the doctrings on the sktime implementation?

  • Which kind of test should I write? Probably a couple of test ensuring the statsmodels implementation agrees with this one.

  • Should I add this model to the examples/01_forecasting.ipynb notebook? Any other place where I should add more examples on this new estimator?

  • Any other feedback or suggestion?

Minor Questions

  • I wanted to add some useful plotting functions to this wrapper. Unfortunately the simple method plot_components fils because a TypeError: float() argument must be a string or a number, not 'Period'. Have you encounter this type of error in the past? It seems the internal plotting functions do not handle Period types.
For all contributions
  • I've added myself to the list of contributors.
  • I've added unit tests and made sure they pass locally.
For new estimators
  • I've added complete doc-strings.
  • I've added the estimator to the online documentation.
  • I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@juanitorduz
Copy link
Contributor Author

In c6180a7 I added the first test to compare with the original implementation from statsmodels (plus an additional docs fix 4129b77)

@juanitorduz
Copy link
Contributor Author

In 68f920d I added the UnobservedComponets estimator into the 01_forecasting notebook (section 2.5)

@juanitorduz
Copy link
Contributor Author

In 884828a I added the model into the online documentation.

I think this draft PR is ready for a first review from the maintainers :)

@juanitorduz juanitorduz marked this pull request as ready for review September 7, 2021 09:18
@juanitorduz
Copy link
Contributor Author

In 95ccf6e I added the doc-strings. Mostly taken from the ones in statsmodels. Again, I am not sure how much information should be added. Feedback is welcome :)

Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

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

Hi @juanitorduz, I had a quick look and I think this looks very good! Only two minor comments. I haven't looked at the notebook changes yet.

sktime/forecasting/structural.py Outdated Show resolved Hide resolved
sktime/forecasting/tests/test_structural.py Outdated Show resolved Hide resolved
Co-authored-by: Markus Löning <markus.loning@gmail.com>
juanitorduz and others added 2 commits September 12, 2021 12:27
Co-authored-by: Markus Löning <markus.loning@gmail.com>
@juanitorduz
Copy link
Contributor Author

juanitorduz commented Sep 13, 2021

@mloning I do not understand why the Azure pipelines are failing in 19093fc 😢 ... any tip?

Update: I waited a day and push an empty commit (8009d52) to trigger the ci/cd and the azure pipelines are not failing 😄 !

mloning
mloning previously approved these changes Sep 15, 2021
Copy link
Contributor

@mloning mloning left a comment

Choose a reason for hiding this comment

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

All looks good to me now! The links now also reader correctly and directly link to the statsmodels online docs! Thanks @juanitorduz

The Azure pipeline failures were due to #1415 but all resolved now.

I'll merge in the next couple of days if nobody else comments.

@mloning mloning added this to the v0.8.0 milestone Sep 16, 2021
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Great addition!

However, there are - probably accidentally - massive changes to the forecasting notebook!
These should be reverted before we merge.

@juanitorduz
Copy link
Contributor Author

Great addition!

However, there are - probably accidentally - massive changes to the forecasting notebook!
These should be reverted before we merge.

Ups! by looking into https://app.reviewnb.com/alan-turing-institute/sktime/pull/1394/files/ I see that in the master branch the notebook does not have any cell outputs 🙈 . I will fix this.

@juanitorduz
Copy link
Contributor Author

juanitorduz commented Sep 16, 2021

Great addition!
However, there are - probably accidentally - massive changes to the forecasting notebook!
These should be reverted before we merge.

Ups! by looking into https://app.reviewnb.com/alan-turing-institute/sktime/pull/1394/files/ I see that in the master branch the notebook does not have any cell outputs 🙈 . I will fix this.

By looking into the diff and https://app.reviewnb.com/alan-turing-institute/sktime/pull/1394/files/ it seems juanitorduz@f2f939c and juanitorduz@9ff3d3f solved the problem.

Remark (specially for future self XD): I had to clear the notebook output twice: (1) The first one in VSCode (my IDE) which actually changed a bit the notebook structure (json order?) and then (2) restart using Jupyter notebooks (Anaconda), which solved the "high" number of changes issue 😃 !

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Yes, thanks for fixing the notebook.

Unrelated, we may later want to add the automated estimator list to section 2, using all_estimators - though that's better a separate PR.

@fkiraly fkiraly merged commit fd7515f into sktime:main Sep 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Add UnobservedComponents model wrapper (statsmodels)
3 participants