Sktime: [BUG] Absolute to relative ForecastingHorizon

[aiwalter]

Describe the bug

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-5-40dcb829593a> in <module>
      4 y_train, y_test = temporal_train_test_split(y, test_size=120)
      5 fh = ForecastingHorizon(y_test.index[0:5], is_relative=False)
----> 6 fh.to_relative(cutoff=y_train.index[-1])

c:\Users\Martin\Desktop\sktime\sktime\sktime\forecasting\base\_fh.py in to_relative(self, cutoff)
    223                 values = _coerce_duration_to_int(values, unit=_get_unit(cutoff))
    224 
--> 225             return self._new(values, is_relative=True)
    226 
    227     @lru_cache(typed=True)

c:\Users\Martin\Desktop\sktime\sktime\sktime\forecasting\base\_fh.py in _new(self, values, is_relative)
    160         if is_relative is None:
    161             is_relative = self.is_relative
--> 162         return type(self)(values, is_relative)
    163 
    164     @property

c:\Users\Martin\Desktop\sktime\sktime\sktime\forecasting\base\_fh.py in __init__(self, values, is_relative)
    137         if not isinstance(is_relative, bool):
    138             raise TypeError("`is_relative` must be a boolean")
--> 139         values = _check_values(values)
    140 
    141         # check types, note that isinstance() does not work here because index

c:\Users\Martin\Desktop\sktime\sktime\sktime\forecasting\base\_fh.py in _check_values(values)
    105     # check values does not contain duplicates
    106     if len(values) != values.nunique():
--> 107         raise ValueError("`values` must not contain duplicates.")
    108 
    109     # return sorted values

ValueError: `values` must not contain duplicates.

To Reproduce

from sktime.forecasting.all import *
y = load_airline()
y.index = y.index.to_timestamp()
y_train, y_test = temporal_train_test_split(y, test_size=120)
fh = ForecastingHorizon(y_test.index[0:5], is_relative=False)
fh.to_relative(cutoff=y_train.index[-1])

Expected behavior
No exception

Additional context

Versions
Current GitHub master

[mloning]

Thanks for raising the bug.

When I run this I don't get the exception but even worse, a silent error (the computed relative horizon is wrong).

The problem comes down to computing time deltas.

This works as expected:

cutoff = y_train.index[-1]
cutoff
>>> Timestamp('1950-12-01 00:00:00', freq='MS')

This also works but returns time deltas in days rather than the months what we would like based on the "MS" (month start) frequency:

values = fh.to_pandas() - cutoff
values
>>> TimedeltaIndex(['31 days', '62 days', '90 days', '121 days', '151 days'], dtype='timedelta64[ns]', name='Period', freq=None)

Trying to convert this into months doesn't seem to work:

(values / pd.Timedelta(1, "MS"))
>>> Float64Index([2678400000.0, 5356800000.0, 7776000000.0, 10454400000.0, 13046400000.0], dtype='float64', name='Period')

Somewhat confusingly "MS" now refers to milliseconds as it seems:

pd.Timedelta(1, "MS")
>>> Timedelta('0 days 00:00:00.001000')

Now this would work:

(values / np.timedelta64(1, "M")).astype(int)

But relies on rounding and may fail for long horizons and still requires to convert "MS" to "M" and probably a few other frequency conversions too.

Alternatively:

fh.to_pandas().to_period("M") - cutoff.to_period("M")

But again requires to know about "MS" meaning "M" here.

Working with pd.PeriodIndex is somewhat easier when it comes to time deltas but also more restricted in terms of other functionality.

Not sure how to best address this, any ideas?

[mloning]

Forcing a conversion to pd.PeriodIndex internally whenever we need to do arithmetics with pd.DatetimeIndex may be the best option (e.g. subtraction of the cutoff point when going from absolute to relative representation). This requires that the pd.DatetimeIndex is regular and that the frequency is given or can be inferred which seems like a reasonable assumption for the methods we currently support. This way we avoid having to deal with pd.Timedeltas altogether.

Something like this could work:

def _coerce_offset_index_to_int_index(index):
    return pd.Int64Index([offset.n for offset in index])

def _check_freq(values):
    # check if values.freq is given

values = self.to_pandas()

if isinstance(values, pd.DatetimeIndex):
    freq = _check_freq(values)
    try:
        # coerce to pd.Period for easier arithmetics
        absolute = values.to_period(freq)
        cutoff = cutoff.to_period(freq)

    except ValueError:
        #raise a more informative error message

    # compute integer-valued relative fh 
    relative = absolute - cutoff
    integers = _coerce_offset_index_to_int_index(relative)

[aiwalter]

@mloning I had a look into it and I think the best way is what you wrote here:

fh.to_pandas().to_period("M") - cutoff.to_period("M")

So basically what you already said, when doing fh.to_relative() then just converting it to a PerdiodIndex first in case it is a DatetimeIndex.
The good thing is that pandas can convert the DatetimIndex also just like this:

from sktime.forecasting.all import *
y = load_airline()
y = y.to_timestamp()
cutoff = y.index[-1]
y.index.to_period()

and it works without giving the freq="M" argument:

PeriodIndex(['1949-01', '1949-02', '1949-03', '1949-04', '1949-05', '1949-06',
             '1949-07', '1949-08', '1949-09', '1949-10',
             ...
             '1960-03', '1960-04', '1960-05', '1960-06', '1960-07', '1960-08',
             '1960-09', '1960-10', '1960-11', '1960-12'],
            dtype='period[M]', name='Period', length=144, freq='M')

The bad thing is that to_period() does not work for a single Timestamp like below. I think I will raise this as an issue to pandas, as this is should then also work imho.

cutoff.to_period()

```python

AttributeError Traceback (most recent call last)
pandas_libs\tslibs\period.pyx in pandas._libs.tslibs.period.freq_to_dtype_code()

AttributeError: 'pandas._libs.tslibs.offsets.MonthBegin' object has no attribute '_period_dtype_code'

The above exception was the direct cause of the following exception:

ValueError Traceback (most recent call last)
in
1 cutoff = y.index[-1]
----> 2 cutoff.to_period()

pandas_libs\tslibs\timestamps.pyx in pandas._libs.tslibs.timestamps._Timestamp.to_period()

pandas_libs\tslibs\period.pyx in pandas._libs.tslibs.period.Period.__new__()

pandas_libs\tslibs\period.pyx in pandas._libs.tslibs.period.freq_to_dtype_code()

ValueError: Invalid frequency: {0}

So I have now a good workaroud to convert the `Timestamp` to a `Period` without including a hardcoded `freq` value or other if statements:
```python
date = pd.DatetimeIndex([cutoff], freq=cutoff.freq)
cutoff = date.to_period()[0]
cutoff

This results in Period('1949-01', 'M'). 🎉 So now we ca do the delta calculation as usual with the PeriodIndex in order to get the relative fh.
What do you think? I can try to implement it.

[mloning]

Hi @aiwalter - yes that sounds like a good plan. Would really appreciate a PR! Even if it doesn't work always, I'd rather have it fail at the to_period() step than what we have currently where it silently produces errors.

[mloning]

There's another issue: pd.DatetimeIndex looses the freq information if the index is no longer regular (e.g. when it has gaps after slicing). Indices with less than 3 items also don't carry the freq information. This makes it hard to rely on it for arithmetic operations.

Some forecasters (e.g. pipelines) may work with irregular time indices (e.g. predictions for specific steps of the forecasting horizon which are passed to the inverse transformation) but will no longer find the freq information on the index.

index = pd.date_range("01-01-2021", periods=10, freq="D")
index.freqstr
>>> "D"

index[[1, 2]].freqstr
>>> None

[aiwalter]

if you do index[:5] it works! Seems to be another pandas issue.

[mloning]

Yes I asked about it on their Gitter: https://gitter.im/pydata/pandas

The issue is we often work with incomplete indices (e.g. forecasting horizon or windows in temporal CV), not sure how to best handle that yet.

[aiwalter]

I was thinking a bit about the bigger picture of how the class ForecastingHorizon is sructured, the way predict() is used and which index types are accepted in fit(). I guess you had some ideas in mind when designing this and allowing different index types, slicing etc. However, I think this makes it a bit vulnerable as the fh became sth like the backbone of sktime. If we would be more restrictive in what we allow, things would be more stable I guess. I had in mind to completely restructure and simplifying it to e.g. the following limitations:

• Data: Accepting only DatetimeIndex (or maybe additionally RangeIndex (like 0,1,2,3,4,5..) which is the dafault index of DataFrame). The later could be internally replaced with a dummy DatetimeIndex so that we internally only deal with one index type. We could also accept all like now, and replace them with DatetimeIndex also internally.

• Predict function: We could only accept the following way to predict, which is mainly based on the freq value of the index:
  1) Allow predict() to just return the next step
  2) Allow predict([1,2,3,4,5])
  3) Allow predict([-3,-2,-1,0,1,2,3])
  4) Maybe allow predict([1,3,10]), not sure on this. Actually doing predictions like this is very unlikely to be used by users.
  5) Allow predict(10) to get next 10 data points
  6) Prohibit predict(-10) or related
  7) Prohibit predictions on index values, just allow int or [int]

• ForecastingHorizon: This class would not be needed any more and could be replaced by an argument fh of type int or [int]

This is just an idea, and some point might be solved better. I am also fine with keeping things like it is now. Would appreciate your feedback on this @mloning !

[mloning]

Some of the rationale for the forecasting horizon can be found here: https://github.com/sktime/enhancement-proposals/blob/master/steps/01_forecasting_api/01_forecasting_api.md

We actually started without the ForecastingHorizon class but realised that we need some abstraction to handle the different representations. Both relative and absolute representations are commonly used in practice. In addition, different libraries require different representation (e.g. compare pmdarima and fbprophet). Having a forecasting horizon class helps us manage these representations. Distinguishing between in-sample and out-of-sample horizons is another problem that you need to handle somehow.

I agree about the simplification. The problem is that pd.DatetimeIndex has more functionality than pd.PeriodIndex but is less reliable for arithmetic computations and seems to forget about freq in some cases.

Regarding your points:

• Accepting only pd.DatetimeIndex and pd.RangeIndex seems reasonable, but most issues come from pd.DatetimeIndex.
• As to the predict interface:
• We could add this as the default argument if no horizon is passed
• That works currently - internally it's simply cast into a ForecastingHorizon
• That works currently too
• Many forecasters generate predictions recursively, i.e. they generate predictions for all previous steps to predict the next step (e.g. ARIMA), however some forecasters don't do that (e.g. "direct" strategy when reducing forecasting to regression where we train a separate model for each step in the horizon). To avoid needless computations, we need to be able to specify the exact steps we want to predict.
• Given the point above, predict(10) wouldn't be the same as predict([10]), I think it's preferable to avoid the confusion over the convenience of not having to type the square brackets in this case
• How would you specify in-sample predictions?
• What do you mean by index values?

Hope this makes our thinking a bit clearer, always happy to be convinced of something else!

[aiwalter]

I think most issues come from:

• converting type of index
• converting fh absolute to relative and vice versa
• slicing index

This is why it would be more stable to just accept..

• one index type (at least use one type interanally). If there is a dependency package that uses a different type, we could wrap that anyway just for this specific package.
• only relative fh
• only continuous fh lists and not sth like in 4), so no slicing

Just few points:
1) Yes we could just add this anyway 👍🏻
5) I agree that acceptiong int might be confusing in combination with list, most likely it would be best to just accept continuous [int] or predict() for one step ahead.
6) Insample would be done by predict([-3,-2,-1]) e.g.
7) I meant using e.g. ForecastingHorizon(['2018-01-01', '2018-01-02', '2018-01-03'], dtype='datetime64[ns]', freq='D', is_relative=False), so basically just accepting relative horizons and no absolute.

I mean as you said, almost all things (inspite 1) are already possible with ForecastingHorizon from user perspective , so that is the good news :)