skforecast icon indicating copy to clipboard operation
skforecast copied to clipboard

Bad error handling when Index is neither RangeIndex nor DateIndex

Open Sergio-Quijano-Stratesys opened this issue 1 year ago • 2 comments

Scope: in the utils.py file, in the line:

https://github.com/JoaquinAmatRodrigo/skforecast/blob/270dcb26923eec62f8be827422de30a16bfccbd4/skforecast/utils/utils.py#L1190

If we have a dataframe with a pandas index that is neither RangeIndex nor DateIndex, we get the following error:

UnboundLocalError: local variable 'new_index' referenced before assignment

That is because there is no else clause in the first inner branch of if statement, and thus, no new_index variable is created. This is easily fixable with something like:

  if isinstance(index, pd.Index):
      if isinstance(index, pd.DatetimeIndex):
          new_index = pd.date_range(
              start=index[-1] + index.freq, periods=steps, freq=index.freq
          )
      elif isinstance(index, pd.RangeIndex):
          new_index = pd.RangeIndex(start=index[-1] + 1, stop=index[-1] + 1 + steps)
      else:
          raise Exception("Index must be of type 'RangeIndex' or 'DateIndex'")
          # Also could be:
          # `new_index = pd.RangeIndex(start=0, stop=steps)`
  else:
      new_index = pd.RangeIndex(start=0, stop=steps)

  return new_index

As you can see, either you raise an informative error to the user or apply the default behaviour. I am happy to open a PR with the fix if you tell me which alternative you prefer.

Hi @Sergio-Quijano-Stratesys, Thanks for pointing this out. I agree that there should be an additional else in the inner if condition. I like the one with the raise exception. (@JavierEscobarOrtiz what do you think?)

Did you find this bug using the skforecast API or by using this function directly? As far as I know, we only use the expand_index function in places where the input should be DatetimeIndex with frequency or RangeIndex.

Sure! We would appreciate if you could send a PR against the 0.12.x branch.

JoaquinAmatRodrigo avatar Feb 06 '24 13:02 JoaquinAmatRodrigo

Thank you @Sergio-Quijano-Stratesys!

I like the one with the raise exception as well. Please, include it as a TypeError.

JavierEscobarOrtiz avatar Feb 06 '24 15:02 JavierEscobarOrtiz

Issue fixed in skforecast 0.12.0, thanks @Sergio-Quijano-Stratesys

JavierEscobarOrtiz avatar May 06 '24 07:05 JavierEscobarOrtiz