Skip to content
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

Make DatetimeIndex timezone naive for compatibility with xarray/numpy engine #1343

Merged
merged 5 commits into from
Nov 10, 2022

Conversation

madtoinou
Copy link
Collaborator

@madtoinou madtoinou commented Nov 8, 2022

Fixes #1052 and #648. Xarray relies on numpy engine, which is not timezone aware. Causes issues for plotting and resampling.

Summary

When constructing a TimeSeries from a pd.DataFrame or a pd.Series with a DatetimeIndex containing timezone information, this attribute is dropped without conversion using tz_localize(None) (conversion can causes shift in the timestamps, making the index invalid). The timezone information can be recovered by calling ts.time_index.tz_localize(original_tz) prior to exporting the data.

Other Information

The DatetimeIndex not used as index are silently converted to UTC by the pd.DataFrame constructor without saving the timezone information. Thus, timezone aware dataset should use the time range as index to take advantage of the bugfix and avoid the error raised by the TimeSeries constructor due to index invalidity.

In order to have the desired x-axis when plotting the TimeSeries (with a timezone unaware time_index due to the bugfix), the user can simply leverage the matplotlib.pyplot rcParams['timezone'] parameter to automatically convert the time axis back to the original timezone.

…upported by xarray): timezone information is removed without convertion. added associated tests
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2022

Codecov Report

Base: 93.89% // Head: 93.88% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (34bdec4) compared to base (1547d5b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1343      +/-   ##
==========================================
- Coverage   93.89%   93.88%   -0.01%     
==========================================
  Files          78       78              
  Lines        8516     8510       -6     
==========================================
- Hits         7996     7990       -6     
  Misses        520      520              
Impacted Files Coverage Δ
darts/timeseries.py 92.34% <100.00%> (-0.02%) ⬇️
...arts/models/forecasting/torch_forecasting_model.py 87.08% <0.00%> (-0.06%) ⬇️
darts/models/forecasting/block_rnn_model.py 98.24% <0.00%> (-0.04%) ⬇️
darts/models/forecasting/nhits.py 99.27% <0.00%> (-0.01%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I just have suggestions for slightly better warning messages, and also to remove the print() statements.

darts/timeseries.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
darts/timeseries.py Outdated Show resolved Hide resolved
Copy link
Contributor

@hrzn hrzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@hrzn hrzn merged commit a0ebdfd into master Nov 10, 2022
@madtoinou madtoinou deleted the fix/timezone-aware-index branch March 12, 2023 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

timezones make time index of wrong type
3 participants