Generate time lagged ensemble with deterministic input#2316
Generate time lagged ensemble with deterministic input#2316gavinevans wants to merge 4 commits intometoppv:masterfrom
Conversation
…a cube without a realization coordinate.
improver/utilities/time_lagging.py
Outdated
| @@ -19,6 +20,32 @@ | |||
| class GenerateTimeLaggedEnsemble(BasePlugin): | |||
| """Combine realizations from different forecast cycles into one cube""" | |||
There was a problem hiding this comment.
To be explicit, please could you add some comments on how the realization coordinate points are handled both when no realization coordinate is present on any cubes and when some cubes have realization coordinates and others don't. Some supporting examples would help.
There was a problem hiding this comment.
I've extended this docstring.
improver/utilities/time_lagging.py
Outdated
There was a problem hiding this comment.
Following the 'Fail fast' principle, this should be moved above the use of the add_realization_coord method.
There was a problem hiding this comment.
I've left this, as I think that I'd prefer the case where a single cube is input, with this cube then being returned, but with a realization coordinate added, to have precedence over the case where the times are compared between multiple cubes, which could result in a failure.
improver/utilities/time_lagging.py
Outdated
There was a problem hiding this comment.
| # Take all the realizations from all the input cubes and |
There was a problem hiding this comment.
I've refactored code on surrounding lines.
| input_cubelist = iris.cube.CubeList([cube1, cube2]) | ||
| result = GenerateTimeLaggedEnsemble().process(input_cubelist) | ||
| self.assertIn("realization", [coord.name() for coord in result.coords()]) | ||
| np.testing.assert_array_equal(result.coord("realization").points, [0, 3, 4, 5]) |
There was a problem hiding this comment.
Although this passes, is this desireable? Would we instead want to ensure that the points are monotonically increasing with constant stride (i.e., [0, 1, 2, 3, 4, 5] rather than [0, 3, 4, 5])? I'm wondering what downstream code expects
There was a problem hiding this comment.
The case of mixed realizations is probably least likely to be actively used, but I've added an option to rebadge the realizations, rather than do this automatically, in case the numbers used within the input realizations do have meaning.
gavinevans
force-pushed
the
mobt_1078_time_lagged_deterministic
branch
from
012b591 to
d579516
Compare
maxwhitemet
left a comment
maxwhitemet
left a comment
There was a problem hiding this comment.
Thanks @gavinevans. Happy with the changes made. Approved 👍
Kat-90
left a comment
Kat-90
left a comment
There was a problem hiding this comment.
Only a couple of minor comments.
|
|
||
| def test_single_cube(self): | ||
| """Test only one input cube returns cube unchanged""" | ||
| """Test only one input cube returns the cube unchanged and raises a warning.""" |
There was a problem hiding this comment.
Maybe this should be changed to something more specific now because you can now change a single input cube by passing in a deterministic one. The warning would also change slightly.
Something like "Only single cube input with realization coord already present, so time lagging will have no effect."?
It might be worth adding a warning to the deterministic cube too that says "Only single deterministic cube input, so realization coord added but time lagging will have no effect."
|
|
||
| def test_single_cube_without_realization(self): | ||
| """Test single input cube without realization coordinate gets one added.""" | ||
| # Remove realization coordinate |
There was a problem hiding this comment.
If my above comment makes sense then we would add a warning to this unit test too.
| np.testing.assert_array_equal(result.coord("realization").points, [0, 1]) | ||
|
|
||
| def test_mixed_cubes_with_and_without_realization(self): | ||
| """Test mixing cubes with and without realization coordinates.""" |
There was a problem hiding this comment.
Could we add what we expect to happen in the docstring like the others. I think it's that the cube without a realization coord gets one sequentially starting from 0.
3 participants
Addresses https://github.com/metoppv/mo-blue-team/issues/1078
Description
This PR makes a minor update the GenerateTimeLaggedEnsemble plugin to support providing deterministic input (i.e. input without a realization coordinate), so that a time-lagged ensemble can be created using deterministic inputs.
Testing: