MOBT-689 (Pt2): Spot forecast subsetting using a neighbour cube #2002
Conversation
…g a neighbour cube via the spot-extraction CLI / SpotManipulation plugin.
…forecast to the sites defined in a neighbour cube with valid IDs.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2002 +/- ##
==========================================
+ Coverage 98.39% 98.42% +0.02%
==========================================
Files 124 126 +2
Lines 12212 12417 +205
==========================================
+ Hits 12016 12221 +205
Misses 196 196 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks @bayliffe 👍
I've added minor comments, with the most significant comment being related to the handling of lapse rate correction, given that spot forecasts are now accepted as input to the spot extract step.
| lapse_rate_cube = forecast.copy() | ||
| with pytest.raises(NotImplementedError, match=msg): | ||
| SpotManipulation(subset_coord="wmo_id", apply_lapse_rate_correction=True)( | ||
| [forecast, lapse_rate_cube, neighbour_cube] |
There was a problem hiding this comment.
I can't see a test for the code returning the whole cube untouched if subset_coord is None. I think this is worthy of a test because it's in a specific condition of the code, but I c 8000 an't tell if it's automatically tested for within one of the other tests.
There was a problem hiding this comment.
Does this test cover it, or are you looking for something different?
There was a problem hiding this comment.
Also, please remember to check out the acceptance test data:
metoppv/improver_test_data#46
There was a problem hiding this comment.
Also, please remember to check out the acceptance test data: metoppv/improver_test_data#46
This is what I did, but I rebased with master to get all the tests to pass. There has been an addition of a bias correction KGO since you opened the branch.
Unfortunately the link to the test you're trying to show me doesn't work.
There was a problem hiding this comment.
Does this test cover it, or are you looking for something different?
Yes it does, thanks, I'd missed that.
…ppv#2002) * Add basic functionality to allow subsetting a spot-forecast cube using a neighbour cube via the spot-extraction CLI / SpotManipulation plugin. * Add comment * Add unit tests for spot forecast subsetting using the SpotManipulation plugin. * Additional tests for spot subsetting. * Add handling of None IDs and an associated test. * Add handling for case where no spot forecasts remain after the subsetting. * Add an acceptance test that demonstrated subsetting an existing spot forecast to the sites defined in a neighbour cube with valid IDs. * Raise exception if lapse rate adjustment is attempted on existing spot forecast. * Remove test I added for checking.
Allows a user to subset an existing spot-forecast via the spot-extraction CLI using a neighbour cube. This will allow us to apply a verification neighbour cube to the end-of-chain forecasts in the preverspot steps to produce a much smaller and controlled set of sites for verification. It removes the need for the preverspot step in the suite to be aliased to different commands for different steps in the chain.
This subsetting is performed using a coordinate selected by the user, with the doc-strings indicating this should be an ID coordinate (e.g. wmo_id or met_office_site_id), rather than e.g. altitude. I have not designed this to handle float comparisons, nor constraining on multiple coordinates as would be required if sites were to be extracted using latitude + longitude.
Acceptance test data: metoppv/improver_test_data#46
Testing: