MOBT-639: Temporal interpolation of period diagnostics #1982
Conversation
…w interpolated values.
… Add bounds to time and forecast period coordinates of interpolated outputs if the inputs are period diagnostics.
…ed to ensure the temporally interpolated values give the same total over the whole period as the original inputs.
…cover the entire period of the input.
…n of the daynight method to cubes that have been interpolated to multiple times, allowing for other coordinates as well. Style fixes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1982 +/- ##
==========================================
- Coverage 98.40% 98.39% -0.01%
==========================================
Files 124 124
Lines 12125 12196 +71
==========================================
+ Hits 11931 12000 +69
- Misses 194 196 +2 ☔ View full report in Codecov by Sentry. |
…mulation diagnostics.
…h other CLIs like combine.
|
Note I have resolved conflicts this morning after merging into master the fix to the various black-formatting artefacts. Some of those fixes were in content removed in this PR, so git did not know how this PR would merge into master. |
bayliffe
added
the
BoM review required
There was a problem hiding this comment.
Hi @bayliffe, I've had a look over the improver portion of this PR and have some comments. I'll work through the tests tomorrow, but based on what I have seen here I think these changes make sense and are a good addition to the interpolation CLI. I'll aim to respond to any feedback and review the remainder of the PR by end of tomorrow so that we can close this one off.
|
@benowen-bom as you've still got to look at the unit tests it is worth being aware that a great deal of the change is simply rewriting the existing tests from IrisTest style to pytest style. I've done this as a rule when I come to significantly edit unit tests as pytests is our preferred option these days. There is a commit point in the history at the point at which I had finished rewriting the existing unit tests in the pytest style without adding any new tests. I'm not sure if you will find this helpful, but it might make clearer what's actually new and what's just a rewrite of what was there already. |
|
Good spot on the extra check. I've got time interpolation blindness at this point. I've made all of your suggested changes; could you maybe have asked for something daft that I could have disputed? I look like a right pushover :-) |
I'll be sure to add something daft next time! All looks good to me now, so happy to approve :-) |
* Modified to return the later of the two input cubes along with the new interpolated values. * Add identification of period diagnostics using time bounds on inputs. Add bounds to time and forecast period coordinates of interpolated outputs if the inputs are period diagnostics. * Add renormalisation for period accumulations. These need to be adjusted to ensure the temporally interpolated values give the same total over the whole period as the original inputs. * Add a check to ensure the guessed bounds of the interpolated outputs cover the entire period of the input. * Partial rewrite of unit tests to use pytest. * All existing unit tests for temporal interpolation converted to pytest. * Style fixes. * Unit tests for new functionality added. Fixed a bug in the application of the daynight method to cubes that have been interpolated to multiple times, allowing for other coordinates as well. Style fixes. * Enforce float type on renormalised accumulations. * Add acceptance tests for temporal interpolation of period data. * Add an additional test to check for overlapping period diagnostics. * Add pass through if only target is the trailing input cube. * Fix up multi-realization accumulation handling and add test coverage. * Improve comment. * Add unit tests to cover unequal length inputs when interpolating accumulation diagnostics. * Modify arguments from maximum and minimum to max and min to align with other CLIs like combine. * Review changes. * Second review changes. * Additional check within plugin. Added bounds checking to unit tests.
Adds options for temporally interpolating period diagnostics (maxima, minima, accumulations). Ensures sensible metadata are returned.
Modifies the outputs returned. These were previously only the times to which data were interpolated, but following this change the output includes these interpolated times and the later of the two inputs cubes. This allows this later input, which for a period represents the preceding period, to be modified as well to reduce the period to that expected in the interpolated outputs. A result of this change is that in a long sequence of lead-times passed in, the first lead-time in the sequence is not made available in any output from the temporal interpolation.
Testing:
Further details: https://github.com/metoppv/mo-blue-team/issues/639
Acceptance test data: metoppv/improver_test_data#43