skforecast
skforecast copied to clipboard
Bad error handling when Index is neither RangeIndex nor DateIndex
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.
Thank you @Sergio-Quijano-Stratesys!
I like the one with the raise exception as well. Please, include it as a TypeError
.
Issue fixed in skforecast 0.12.0, thanks @Sergio-Quijano-Stratesys