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] ensure Prophet can deal with PeriodIndex #2475

Closed
fkiraly opened this issue Apr 14, 2022 · 4 comments · Fixed by #3995
Closed

[ENH] ensure Prophet can deal with PeriodIndex #2475

fkiraly opened this issue Apr 14, 2022 · 4 comments · Fixed by #3995
Labels
good first issue Good for newcomers module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting

Comments

@fkiraly
Copy link
Collaborator

fkiraly commented Apr 14, 2022

Currently, Prophet errors out when given data frames with PeriodIndex, due to the interfaced facebook prophet not liking that index (and catch statements in our interface).

We may want to add some intermediate functionality to enable Prophet to also deal with PeriodIndex? In that case, I'd simply convert it to the first time stamp in a period, then apply Prophet as is, and at the end convert back to PeriodIndex.

Good first issue, the one non-obvious thing perhaps is that the places to look at includes the _ProphetAdapter class as well.

@fkiraly fkiraly added good first issue Good for newcomers module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting labels Apr 14, 2022
@aiwalter
Copy link
Contributor

back in the days we have decided to accept only pd.DatatimeIndex and not do the conversion as not for all freq values you can convert back and forth without loss of freq information. I even opened 2 bug issues for this on pandas GitHub which are still open:

pandas-dev/pandas#38914
pandas-dev/pandas#38885

So we might have to implement a custom dummy index conversion:

@fkiraly
Copy link
Collaborator Author

fkiraly commented Apr 14, 2022

back in the days we have decided to accept only pd.DatatimeIndex and not do the conversion

But already our example datasets violate this input assumption, e.g., load_longley used above.
This is programmed to cause user frustration...

@aiwalter
Copy link
Contributor

yes, we should actually open an issue at prophet about this

@fkiraly
Copy link
Collaborator Author

fkiraly commented Apr 14, 2022

with last release over a year ago? Can try, but sounds unlikely...
We'll probably need to find a way to work around it

@fkiraly fkiraly added this to Should have in Release v0.14.0 Jul 3, 2022
fkiraly added a commit that referenced this issue Dec 31, 2022
… not working with non-integer forecast horizon (#3995)

This PR adds functionality to the `Prophet` interface to also allow `pd.PeriodIndex`. Fixes #2475 and also fixes #3537.

Also tests:
* testing `Prophet` functionality for `RangeIndex` and `PeriodIndex` in exogeneous and endogeneous data
* testing failure case in #3537
Workstream: forecasting and series transformers automation moved this from To do to Done Dec 31, 2022
fkiraly added a commit that referenced this issue Feb 12, 2023
This removes the exception from testing `Prophet` with `pd.DatetimeIndex` or `pd.PeriodIndex`.

Recent changes in `Prophet` and framework to fix the issue #2475 also fix the failures in the excepted test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers module:forecasting forecasting module: forecasting, incl probabilistic and hierarchical forecasting
2 participants