Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #35 +/- ##
==========================================
+ Coverage 98.46% 98.50% +0.04%
==========================================
Files 7 7
Lines 65 67 +2
Branches 8 8
==========================================
+ Hits 64 66 +2
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@taylor13 @JamesAnstey thoughts welcome |
|
Thanks @znicholls for implementing this, looks good to me. A quibble: since the cell methods for these variables has "time: minimum within days time: mean over days" (or similar for max) would keying off just the cell methods be more general, rather than using time4? In case in future the same cell methods is used for a time averaging period that's not a month (as time4 is specified for in the DR). But for the AFT DR it doesn't matter since there are only 5 affected variables and all are monthly with time4 as their time dimension. |
Yes probably. At the moment I'm just blindly implementing Table F1 from whatever version of Karl's draft I last looked at, but I'm sure Karl would be happy to talk about the logic of the underlying algorithm too (which can then appear here, with fun thoughts like how careful we need to be about the ordering of the operations in the cell methods and how to match them robustly). Timing of all that up to you too |
|
To add my 2 cents ... currently and I think in the future CMIP phases, "time4" will invariably imply that the cell methods includes either "time: minimum within days time: mean over days" or "time: maximum within days time: mean over days" (or similar for max)", so I think the current test will remain robust. The "time_label" should not care about the reporting frequency, so the same method will apply for a mean over a year or a decade or a week. All those will have "time4" as a dimension (just as "time1" indicates point sampling, independent of frequency). |
|
Ok gotcha, thanks @znicholls & @taylor13. So it sounds like in future the VR definition of time4 would be updated to not refer specifically to a monthly mean, to accommodate other possible averaging periods. But for the current DR it's perfectly fine. |
|
Ok I think we're all good then. @eahoegner @NiklasSchwind @TessaM97 FYI more updates. I'll merge and release now |
|
@JamesAnstey I mean it couldn't hurt to update the description of time4 in the data request to be consistent with Table F1, which does not restrict it to a single frequency, but it's not essential, I guess. |
+1. For context, I think most (all?) the values in the VR at the moment are just placeholders. Cleaning up all these definitional things would be something for when we go full steam on the VR |
Description
Checklist
Please confirm that this pull request has done the following:
changelog/