-
Notifications
You must be signed in to change notification settings - Fork 91
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
MOBT-799: CLI and plugin for subdividing duration diagnostics #2060
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2060 +/- ##
==========================================
+ Coverage 98.39% 98.41% +0.01%
==========================================
Files 124 134 +10
Lines 12212 13227 +1015
==========================================
+ Hits 12016 13017 +1001
- Misses 196 210 +14 ☔ 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.
Thanks @bayliffe 👍
I've added some minor comments below.
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.
Thanks for the updates, @bayliffe 👍
I've added a comment related to one typo.
…to the plugin as required to address issues found when writing the tests.
…ing multiple realizations.
Co-authored-by: gavinevans <gavin.evans@metoffice.gov.uk>
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've read through and understood the code and tests, the comments in the unit tests were really helpful.
The tests on my local branch all pass.
I have requested some changes to the doc strings based on my understanding of it and one suggestion for another test.
@Kat-90 I've responded to the review. Please take a look at the acceptance test data too. |
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.
Thanks for the changes. I've run the new tests, LGTM.
Relates to: https://github.com/metoppv/mo-blue-team/issues/767
Adds a plugin to divide up duration diagnostics into shorter periods. Night or day masking can be applied to prevent spreading diurnally tied diagnostics into periods when they should not occur.
As an example, splitting 3-hours of sunshine duration, held in a 6-hour period cube, into two 3-hour period cubes. If the day/night terminator for a grid point being considered fell across the site at exactly the mid-point of the original 6-hour period, all 3-hours of sunshine duration will be allocated to the 3-hour period that falls in day time. If this were the mid point of the day the duration would simply be divided up equally into the two shorter periods.
This code cannot take account of weather impacts, e.g. cloud on sunshine duration. It is intended for use in constructing daily summaries (24-hour periods), but making these available at 3-hourly intervals to serve global time zones. The method will introduce errors, and limits the accuracy of the data to the accuracy of our simple 2-D day/night mask. These are compromises made to serve global time zones when the input data is only available at 6-hourly intervals from ECMWF.
Testing: