-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fix mda8 misalignment #1322
Fix mda8 misalignment #1322
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main-dev #1322 +/- ##
============================================
- Coverage 78.97% 78.96% -0.01%
============================================
Files 136 136
Lines 20812 20813 +1
============================================
Hits 16436 16436
- Misses 4376 4377 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
I'm not sure about the number of valid 8-hour means are correct for the first MDA8
pyaerocom/stats/mda8/mda8.py
Outdated
new_arr = arr.rolling(time=8, min_periods=6).mean() | ||
# Xarray labels the data left, while we want it to be labeled right, so we add 8h to | ||
# the time index. | ||
new_arr["time"] = new_arr.get_index("time") + pd.Timedelta("8h") |
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.
Why is the new_arr.get_index("time")
needed?
Can it be written as
new_arr["time"] += pd.Timedelta("8h")
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.
+= Doesn't work.
This is the documented way of doing time offset arithmetic: https://github.com/pydata/xarray/pull/8479/files
Alvaro is on leave, and his change requests have been addressed.
Change Summary
Per discusion with @avaldebe, there is an issue where the mda8 calculation relies on undocumented xarray behaviour. This PR aims to make arguments explicit where possible as well as testing for the undocumented behaviour in xarray so that possible future regressions can be detected. This also fixes an issue where 8 hour average was left labeled instead of rightlabeled, resulting in the mda8 binning to be shifted by 8 hours from the defined window.
Related issue number
closes #1323
Checklist