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

[ENH] Interface statsmodels MSTL - transformer #5125

Merged
merged 23 commits into from Aug 29, 2023
Merged

Conversation

luca-miniati
Copy link
Contributor

What does this implement/fix? Explain your changes.

Interfaces statsmodels MSTL, replaces #4970

For new estimators

  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

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.

Nice, great!

A question, not necessarily a change request:

In the statsmodels example here
https://www.statsmodels.org/devel/generated/statsmodels.tsa.seasonal.MSTL.html

it seems that MSTL returns have as many seasonal components (named Seasonal_[period] in the plot) as there are entries to the iterable periods. Whereas, this transformer seems to add the seasonal components all up?

The result must allow to access these somehow, since the plots displays the components separately. Although there's no clear documentation in statsmodesl that I can find...

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 18, 2023

Another comment, there is also this PR, seems same name?
#4970

It makes sense to have one PR with the transformer, and one (the other one?) with a forecaster - where the forecaster makes forecasts for the seasonal components and the trend component, with defaults being NaiveForecaster(sp=period) (one per period) and TrendForecaster.

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.

It looks like that in this commit:
c5df14c

you accidentally deleted the file containing the MSTL transformer?

@luca-miniati
Copy link
Contributor Author

The result must allow to access these somehow, since the plots displays the components separately. Although there's no clear documentation in statsmodesl that I can find...

I've split the seasonal components as shown in the statsmodels example. Hopefully, this should pass tests, as that weird parsing error seems to be gone.

@luca-miniati
Copy link
Contributor Author

It looks like that in this commit: c5df14c

you accidentally deleted the file containing the MSTL transformer?

Yes, I had moved the file and forgot to add the "new" file. Should be there now under sktime/transformations/mstl

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.

Looks great!

Blocking change request: can you explain to the user in the docstring what happens in transform (and/or fit)?

Non-blocking change requests:

  • stray newline in API reference
  • should there be an inverse transform? That should just sum up the three components back to one, as far as I see.

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! Looks all good!

Now, since you decided to add the _inverse_transform (very useful!) you should ensure the tags are set that turn it on, see above.

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 28, 2023

Ok, the failures seem to be the same as what PR #5166 addresses - that some inverse_transform are not exact, so the test that checks that should not run.

Otherwise all seems fine.

In terms of actions, none needed from your side - I'll update the tag once #5166 is merged, and then all should pass.

@fkiraly fkiraly changed the title [ENH] Interface statsmodels MSTL [ENH] Interface statsmodels MSTL - transformer Aug 29, 2023
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.

I've updated the tags after merging the respective framework PR, so the one outstanding point should now be addressed!

@fkiraly fkiraly merged commit a9800e6 into sktime:main Aug 29, 2023
22 of 24 checks passed
@luca-miniati luca-miniati deleted the MSTL-2 branch August 29, 2023 23:25
yarnabrina added a commit to yarnabrina/sktime-fork that referenced this pull request Sep 3, 2023
…encies

* origin/main:
  [DOC] speed-up tutorial notebooks - deep learning classifiers (sktime#5169)
  [ENH] fixture names in probability distribution tests (sktime#5159)
  [ENH] test for specification conformance of tag register (sktime#5170)
  &SmirnGregHM [BUG] ensure forecasting tuners do not vectorize over columns (variables) (sktime#5145)
  [ENH] VMD (variational mode decomposition) transformer based on `vmdpy` (sktime#5129)
  [ENH] Interface statsmodels MSTL - transformer (sktime#5125)
  [ENH] add tag for inexact `inverse_transform`-s (sktime#5166)
  [ENH] refactor and add conditional execution to `numba` based distance tests (sktime#5141)
  [MNT] move fixtures in `test_dropna` to `pytest` fixtures (sktime#5153)
  [BUG] prevent exception in `PyODAnnotator.get_test_params` (sktime#5151)
  [MNT] move fixtures in `test_reduce_global` to `pytest` fixtures (sktime#5157)
  [MNT] fix dependency isolation of `DateTimeFeatures` tests (sktime#5154)
  [MNT] lower dep bound compatibility patch - `binom_test` (sktime#5152)
  [MNT] test forecastingdata downloads only on a small random subset (sktime#5146)
  [ENH] widen scope of change-conditional test execution (sktime#5147)
  [DOC] update forecasting extension template on `predict_proba` (sktime#5138)
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.

None yet

2 participants