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

[MNT] [ENH] Several updates in direct statsforecast interface estimators #5920

Merged
merged 16 commits into from Feb 17, 2024

Conversation

yarnabrina
Copy link
Collaborator

@yarnabrina yarnabrina commented Feb 9, 2024

Per estimator lower bound

I checked the imports in _get_statsforecast_class for all direct estimators, and first tag in StatsForecast repository where that class is present. Accordingly, updated the python_dependencies tag per concrete estimators as follows:

  1. AutoARIMA: v1.0.0
  2. AutoCES: v1.1.0
  3. AutoTheta: v1.3.0
  4. AutoTheta: v1.3.2
  5. ARCH and GARCH: v1.5.0 (FYI @eyjo)
  6. MSTL: v1.2.0 (FYI @luca-miniati)

Missing credits

  1. @arnaujc91 has contributed significantly to _GeneralisedStatsForecastAdapter by adding support for argument differences across versions, added in the authors.
  2. Both @arnaujc91 and @luca-miniati (author of StatsForecastBackAdapter) were missing from __author__, added there.
  3. Added StatsForecastBackAdapter in __all__.
  4. Author/maintainer information for @eyjo was missing for StatsForecastARCH, added there.

Enhancements

  1. v1.7.1 adds a new phi parameter to AutoETS, added support for the same.
  2. pydocstyle was failing due to missing docstrings, added one liner summaries (unlikely to be user facing methods).

MSTL changes

  1. StatsForecastMSTL was initialising the attributes after call to super().__init__, which was causing AttributeError in tests that check for presence of properties. This PR moves those initialisations prior to that call. FYI @luca-miniati.
  2. Disabled probabilistic capability of StatsForecastMSTL, as it was not working as reported by @EliasKng in [ENH] expose fitted params for statsforecast forecasters via get_fitted_params #5720. FYI @luca-miniati.

Update of lower/upper bounds

  1. No existing direct estimator support statsforecast<1.0.0 with their current imports, not even AutoARIMA, updated lower bound from 0.5.2 to 1.0.0. Previous adapter by @FedericoGarza was importing from a different location. v0.5.2 was released in March 2022, and v1.0.0 was released in August 2022, so I think chances of affecting users is quite low.
  2. Recently added AutoTBATS needs v1.7.2, so updated upper bound from 1.7.0 to 1.8.0.

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 9, 2024

Missing credits

Apologies, I did a mindless bulk migration - might be my mistake, might be not having been entered properly in the first place.

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 9, 2024

Minor note, StatsForecastBackAdapter should not be user facing, it is used ony as an internal adapter afaik.

@fkiraly fkiraly added maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting enhancement Adding new functionality labels Feb 9, 2024
@yarnabrina
Copy link
Collaborator Author

Minor note, StatsForecastBackAdapter should not be user facing, it is used ony as an internal adapter afaik.

Agreed, but pydocstyle was complaining about this being public, so added docstrings and all just for that. It's still not exposed through init, so hopefully no effect on users.


The failing tests are all from StatsforecastMSTL, especially where anything is trying to use probabilistic predictions. The bug was already reported in #5703, and I think the previous results problem reported in #5586 may also be related. My inference is these failures are not caused by my changes in this PR, because the only effect this PR will have on it is to force running the tests and nothing else.

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 13, 2024

to avoid blocking this PR, can we skip MSTLForecaster for now?
I see two options: setting the capability:pred_int tag to False (this would make sense if none of the methods work), or adding the entire estimator - or just the failing tests - to the set of exceptions in tests._config.

v1.0.0 is the first tag supporting `from statsforecast.models import AutoARIMA`
v1.1.0 is the first tag supporting `from statsforecast.models import AutoCES`
v1.3.0 is the first tag supporting `from statsforecast.models import AutoTheta`
v1.3.2 is the first tag supporting `from statsforecast.models import AutoETS`
v1.5.0 is the first tag supporting `from statsforecast.models import (G)ARCH`
v1.2.0 is the first tag supporting `from statsforecast.models import MSTL`
@yarnabrina
Copy link
Collaborator Author

to avoid blocking this PR, can we skip MSTLForecaster for now? I see two options: setting the capability:pred_int tag to False (this would make sense if none of the methods work), or adding the entire estimator - or just the failing tests - to the set of exceptions in tests._config.

I thought the same, but wasn't sure if it's an acceptable solution. Since you also suggested, doing it for now by combining both options.

  1. Test added specifically in test_statsforecast.py by @sd2k is skipped in _config, plus added a skip decorator with a reason.
  2. Set capability:pred_int and capability:pred_int:insample as False -> this is breaking change, even though it was already not working
  3. Manually set the tags once more in __init__ as _check_supports_pred_int method in _GeneralisedStatsForecastAdapter that @arnaujc91 added overrides class tag during super().__init__()
  4. Skipped test_pred_int_tag tests in _config, as the tag set for StatsForecastMSTL is incompatible with generic capability of _GeneralisedStatsForecastAdapter

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.

Makes sense.

Can I ask a quick question: why did test_statsforecast_mstl previously pass?

Blocking comments:

  • the content of _config affects only suite tests, in the TestAll clasess, so test_statsforecast_mstl wlil have no effect, so there it should be removed
  • I worry that we track nowhere that we need to reactivate the test, or what to do when the issue is fixed. Alternatively, we should record in an issue what exacly to do to reverse once the bug is fixed.
    • in test_statsforecast_mstl, should we maybe instead theck the tag and return None? That would remove one location requiring an explicit reversal.

@yarnabrina
Copy link
Collaborator Author

yarnabrina commented Feb 14, 2024

Can I ask a quick question: why did test_statsforecast_mstl previously pass?

I do not know. Unchecked guess: specific statsforecast version.

I checked locally that running following commands works only if I have statsforecast==1.6.0.

> pytest sktime/forecasting/tests/test_statsforecast.py
> pytest sktime/forecasting/tests/test_all_forecasters.py -k "StatsForecastMSTL"

It fails with 1.5.0 (error is ImportError) and 1.7.0 (error is AttributeError). This is too narrow (just one minor version) supported range in my opinion.

@fkiraly
Copy link
Collaborator

fkiraly commented Feb 14, 2024

I see - back-adaptation is quite tricky, perhaps impossible, if the interfaces do not remain stable

@yarnabrina
Copy link
Collaborator Author

@fkiraly @benHeid @achieveordie please let me know if there are any suggestions for this PR.

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.

Nothing more to do - my request (test skip) has been addressed

@yarnabrina yarnabrina merged commit 63a95f2 into sktime:main Feb 17, 2024
201 checks passed
@yarnabrina yarnabrina deleted the statsforecast-updates branch February 17, 2024 12:29
fkiraly pushed a commit that referenced this pull request Mar 7, 2024
PR #5920 intended to update bounds for `statsforecast` in all dependency
specifications. As noted in #6057, it was done only in `all_extras` and
`all_extras_pandas2`. This PR updates it for the missed single component
optional dependency `forecasting`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality maintenance Continuous integration, unit testing & package distribution module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants