-
Notifications
You must be signed in to change notification settings - Fork 861
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
[timeseries] Fix pandas groupby bug + GluonTS index bug #2420
Conversation
Job PR-2420-28d8f49 is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Job PR-2420-9523ddf is done. |
Job PR-2420-35c3780 is done. |
Job PR-2420-e992f1b is done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -76,7 +77,7 @@ def __iter__(self) -> Iterator[Dict[str, Any]]: | |||
df = self.target_df.loc[item_id] | |||
time_series = { | |||
FieldName.ITEM_ID: item_id, | |||
FieldName.TARGET: df.squeeze().to_numpy(dtype=self.float_dtype), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a performance related change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current code may lead to a bug. If df
has length 1, then df.squeeze()
will be a float
instead of pd.Series
, and calling to_numpy
on this float throws an exception.
There is no clean way to convert a DataFrame to a Series (squeeze
is the only function recommended for this purpose), so we rather do the flattening into 1D after converting to numpy with ravel
.
Description of changes:
sort=False
(related to BUG: df.groupby(sort=False) sorts multi-index-frames pandas-dev/pandas#17537)"2022-01-01 12:00"
,"2022-01-02 12:00"
(frequency is daily), GluonTS will produce forecasts with timestamps"2022-01-01 00:00"
,"2022-01-02 00:00"
, which results in an AssertionError inside theTimeSeriesEvaluator
hereBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.