Fix/bug time series.from group dataframe 2491 #2512
Conversation
darts/tests/test_timeseries.py
Outdated
| }) | ||
| ts = TimeSeries.from_group_dataframe(df, group_cols="group", time_col="time") | ||
| assert isinstance(ts[0].time_index, pd.RangeIndex) | ||
|
|
There was a problem hiding this comment.
Can you add a test for time made out of pd.Timestamps, just to make sure they are converted to the expected type (DatetimeIndex)?
You could also try to leverage the pytest.parametrize functionality to make the test a bit shorter.
Also, would be nice to add a case where the index (either list of int or list of Timestamps) are not ordered, to verify that the sorting of the index behave as expected.
There was a problem hiding this comment.
Thanks for the comments and the corrections, much appreciated. There are some more tests on the group_from function inside "test_timeseries_static_covariates" already, but I agree, it's better to test and unsorted index (and datetimeindex with timestamps).
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2512 +/- ##
==========================================
- Coverage 93.79% 93.78% -0.02%
==========================================
Files 139 139
Lines 14740 14728 -12
==========================================
- Hits 13826 13813 -13
- Misses 914 915 +1 ☔ View full report in Codecov by Sentry. |
typo in function name Co-authored-by: madtoinou <32447896+madtoinou@users.noreply.github.com>
* Included a warning about monotonically increasing index also in the case where time_col is set (analogous to the warning in the case 'time_cole is None and df.index.is_monotonic_increasing)
…ii) an unsorted datetimeindex
* potential speed improvement by case distinction within the time column (need to check this again)
… set_index and casting DatetimeIndex when the time_col is of type datetime
…concerning "from_group_dataframe" are allocated
There was a problem hiding this comment.
Looks great, thanks @AlessiopSymplectic 🚀
Just some last minor comments, then we're ready to merge.
| assert (ts[0].time_index == pd.RangeIndex(3)).all() | ||
| assert (ts[1].time_index == pd.RangeIndex(3)).all() |
There was a problem hiding this comment.
You can write it like this everywhere (and remove the assert in the line above)
| assert (ts[0].time_index == pd.RangeIndex(3)).all() | |
| assert (ts[1].time_index == pd.RangeIndex(3)).all() | |
| assert ts[0].time_index.equals(pd.RangeIndex(3)) |
| assert (ts[0].time_index == pd.RangeIndex(3)).all() | ||
| assert (ts[1].time_index == pd.RangeIndex(3)).all() |
There was a problem hiding this comment.
Also adding a check that the values in the TimeSeries are in the expected order:
You can extract the values with ts[0].values()
Co-authored-by: Dennis Bader <dennis.bader@gmx.ch>
…taframe_2491' into fix/bug_TimeSeries.from_group_dataframe_2491
Checklist before merging this PR:
Fixes #2491
Summary
In the function "from_grou_dataframe()" there was an instance that casted the index as a Datetime index before sorting the index. The line was removed.
A test called "test_grom_group_dataframe()" was added to test_timeseries to verify the intended functionality.