Fix alignment of daily data with inconsistent calendars in multimodel statistics #1212
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1212 +/- ##
==========================================
- Coverage 85.53% 85.51% -0.03%
==========================================
Files 188 188
Lines 9159 9131 -28
==========================================
- Hits 7834 7808 -26
+ Misses 1325 1323 -2
Continue to review full report at Codecov.
|
|
Thanks @Peter9192, that's great. To aid in finishing this up, could you please "do" the checklist, i.e. tick done items and remove or strike out non-applicable items? Also, could you try to put a plainer description, i.e. distinguish between filling missing and removing superfluous days and make clear if and how this relates to the |
|
@Peter9192 thanks a lot for the fix! For the version of recipe_pv_capacity_factor.yml including the multi-model mean using different calendars runs with it. But I wonder if it should without Warning? Or would if it would be even better to only allow this if an additional preprocessor like regrid_time is used? |
|
I think let's roll with it for now (i.e. the release) and revisit the issue of calendars afterward. I did a review of the code here and think it is an improvement, but it does not support 360 day calendars, for instance, just like the old multi-model code did not support it. There are a few more quirks I'd like to talk about and probably go to handling on a day-of-year basis, but for now, let's get this in as a stable platform to base future improvements on. |
I agree that would be good, also see #744. |
|
@katjaweigel, if you are nevertheless happy to move forward with this for now, could you please "Approve" the PR? |
There was a problem hiding this comment.
I like the method to regrid the calendar, but I would at least keep a warning and not replace the warning without note with def _map_to_new_time.
Maybe it would be better to only allow it, if regrid_time is used as preprocessor (in agreement with different horizontal grids, here the multi-model mean can also only be applied if everything is interpolated on a common grid.)?
@zklaus I rather agree that this one could wait, because since the multi-model mean is removed from recipe_pv_capacity_factor.yml I don't think there is any recipe affected by this issue. |
The warning is not removed here; it remains in lines 116ff of
Yes, but let's leave that to #744. |
@zklaus you are right, the warning is there. Sorry I checked the wrong log file! |
|
I edited this so that #1210 stays open for a better long-term solution. |
… statistics (#1212) * Write test to determine behaviour for inconsistent daily calendars * Use nearest neighbour interpolation to align cubes. * remove dask import * use cftime instead of xarray cftime util
Description
Following #1210 this PR implements the following behavior for multimodel statistics, when the input data is a daily frequency with inconsistent calendars:
span='overlap': discard those days that occur only in certain calendars but not in othersspan='full': use nearest-neighbour lookup to fill missing days. Missing dates outside the original date range are masked.This is in contrast to the previous behavior, which was:
span='overlap': Find the first and last day that is present in all datasets; remove everything before the first and after the last day.span='full': Find the very first and the very last day across all cubes; pad every cube with masked out data before and after its actual data to cover the full time range.This only worked for data with the same number of days per year, raising an exception otherwise.
Closes #1198
Link to documentation:
Before you get started
Checklist
It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.
🧪 and 🛠 Documentation is availableTo help with the number pull requests: