Repository navigation
Extract common time points when there is multiple FRT, ensure we are using this as part of a recipe instead of embedded. - #2468
Conversation
Total coverage: 93% (HTML report)Name Stmts Miss Branch BrPart Cover --------------------------------------------------------------------------------------------------- src/CSET/__init__.py 113 4 14 0 97% src/CSET/_common.py 156 0 54 0 100% src/CSET/cset_workflow/app/fetch_fcst/bin/fetch_data.py 117 28 26 0 78% src/CSET/cset_workflow/app/fetch_nimrod/bin/fetch_nimrod.py 81 8 28 11 83% src/CSET/cset_workflow/app/finish_website/bin/finish_website.py 79 2 8 2 95% src/CSET/cset_workflow/app/parbake_recipes/bin/parbake.py 29 0 8 0 100% src/CSET/cset_workflow/app/send_email/bin/send_email.py 25 0 4 0 100% src/CSET/cset_workflow/lib/python/jinja_utils.py 17 0 6 0 100% src/CSET/extract_workflow.py 105 1 26 1 98% src/CSET/graph.py 44 0 14 0 100% src/CSET/operators/__init__.py 89 0 26 0 100% src/CSET/operators/_atmospheric_constants.py 9 0 0 0 100% src/CSET/operators/_colormaps.py 249 3 72 4 98% src/CSET/operators/_stash_to_lfric.py 3 0 0 0 100% src/CSET/operators/_utils.py 232 9 86 7 95% src/CSET/operators/ageofair.py 142 7 64 5 94% src/CSET/operators/aggregate.py 184 8 60 7 94% src/CSET/operators/aviation.py 61 0 18 0 100% src/CSET/operators/collapse.py 162 8 80 3 94% src/CSET/operators/constraints.py 115 7 50 2 93% src/CSET/operators/convection.py 38 4 10 2 88% src/CSET/operators/ensembles.py 27 0 14 0 100% src/CSET/operators/feature.py 44 0 10 0 100% src/CSET/operators/filters.py 67 2 30 0 98% src/CSET/operators/fluxes.py 41 0 10 0 100% src/CSET/operators/humidity.py 135 0 52 0 100% src/CSET/operators/imageprocessing.py 56 0 16 0 100% src/CSET/operators/mesoscale.py 18 0 2 0 100% src/CSET/operators/misc.py 192 6 86 6 96% src/CSET/operators/plot.py 1171 155 438 70 84% src/CSET/operators/power_spectrum.py 156 10 60 8 91% src/CSET/operators/precipitation.py 204 2 92 2 99% src/CSET/operators/pressure.py 41 0 12 0 100% src/CSET/operators/read.py 453 25 200 15 94% src/CSET/operators/regrid.py 153 1 78 2 99% src/CSET/operators/scoreswrappers.py 225 22 74 6 89% src/CSET/operators/temperature.py 121 0 32 0 100% src/CSET/operators/transect.py 63 0 24 0 100% src/CSET/operators/wind.py 46 3 10 2 91% src/CSET/operators/write.py 15 0 6 0 100% src/CSET/recipes/__init__.py 104 0 28 0 100% src/CSET/sample_data/__init__.py 0 0 0 0 100% --------------------------------------------------------------------------------------------------- TOTAL 5382 315 1928 155 93% |
There was a problem hiding this comment.
I think I prefer the approach of adapting the misc operator extract common time points and make it flexible to handle multiple cubes/and or multiple frt, rather than multiple functions in _time_utils that handle specific cases?
Haven't properly reviewed yet, but thought good to flag general approach first so we're on same page
The approach I've taken on #2419 is to adapt misc.py https://github.com/MetOffice/CSET/pull/2419/changes#diff-293d00b74262ce198976adc7667a4b1dd568be13fb8256413b5ceb89910dfadd and just add a new function for multiple cubes, which is largely a wrapper around extract common points which adds a check to ensure forecast reference times are the same (if we know forecast reference time is the same we can use forecast period to extract common time points)
| """Equalise time points across all cubes.""" | ||
| if isinstance(cubes, Cube) or len(cubes) < 2: | ||
| return cubes | ||
| if cubes[0].coords("forecast_reference_time"): |
There was a problem hiding this comment.
will this return true if a cube contains a scalar coordinate forecast_reference_time? I.e. we should check that forecast_reference_time is a dimcoord, that has more than one point. The issue here is you could have a cube list of the same variable but different forecast_reference_times.
| def _extract_common_time_points(base: Cube, other: Cube) -> tuple[Cube, Cube]: | ||
| """Extract common time points from cubes to allow comparison.""" | ||
| # Get the name of the first non-scalar time coordinate. | ||
|
|
There was a problem hiding this comment.
| return _extract_common_time_points_multiple_cubes_no_frt(cubes) | ||
|
|
||
|
|
||
| def _extract_common_time_points(base: Cube, other: Cube) -> tuple[Cube, Cube]: |
There was a problem hiding this comment.
duplication of
CSET/src/CSET/operators/misc.py
Line 471 in 8c2adbd
James Ross (james-a-f-ross)
changed the title
5f96d5d to
4333ae8
Compare
Carol Halliwell (cehalliwell)
left a comment
Carol Halliwell (cehalliwell)
left a comment
There was a problem hiding this comment.
Review of method:
The approach to moving the extraction of common points from operators to the recipe is sound. However, whilst looking at some of the recipes, I did wonder about the order of the calls to the operators when _extract_common_time_points is one of them.
For example, is it correct to calculate the mean on all available points and then extract common points before calling further operators or is it more appropriate to extract the common points in both space and time before calculating the mean so that the meaning is done on only the common points? The order is likely to affect results (and so comparison of old and new methods needs to be done carefully) so something to think about. I think the current order follows the same order as when the extraction of common points is embedded in the operator but is this the correct method?
There are examples in the recipes other than collapsing to the mean that the order of operators needs to be considered. For example, some recipes extract common points over space, run misc.combine_cubes_into_cubelist and then extract common points over time but should the extraction of common points over time follow the extraction of common points in space?
|
Carol Halliwell (@cehalliwell) Thanks for the review. Good to think about order of operators. Do you have an example of e.g the mean before the extract_common_time_points operator. I think since these wouldn't be averaging on the time dimension (but collapsing the space dimension) then the order only matter in terms of efficiency, right? I think on the second point both ways of ordering the operators could work. |
|
An example of the recipe is Unless I'm wrong or overthinking it here, wouldn't it be better practice to extract all the common points in time and space before the main operators? Maybe it wouldn't make a difference to results? |
|
You're right about the first one, it's collapsing in time, so extract_common_time_points is doing nothing there and probably should be before. Good spot, and shows it was a good idea to make it an operator, as this function was being called in misc.difference before. I will change that and look into where else needs to do this. |
|
An example of the other recipe structure where misc.extract_common_time_points appears is |
|
ah yes, actually those are extracting common time points twice basically, made changes |
883077b to
b994670
Compare
|
I'm happy with the changes to the recipes. James Warner (@jwarner8) ready for your review now. |
b994670 to
98e20a1
Compare
…nd removing from scoreswrappers # Conflicts: # src/CSET/operators/scoreswrappers.py
# Conflicts: # src/CSET/operators/aggregate.py # tests/operators/test_scoreswrappers.py
98e20a1 to
e1b0e0e
Compare
James Warner (jwarner8)
left a comment
James Warner (jwarner8)
left a comment
There was a problem hiding this comment.
Looks good, comments mostly tidy-up and additional tests. Given it changes multiple recipes, and we have no end to end workflow testing yet, it would be good to check nothing here breaks a full workflow (possibly using a RAL4 config - I can share).
| return array_min, array_max, array_mean | ||
|
|
||
|
|
||
| def get_time_coord(cube: Cube) -> str | None: |
There was a problem hiding this comment.
| def get_time_coord(cube: Cube) -> str | None: | |
| def get_time_coord(cube: Cube | CubeList) -> str | None: |
If cube could be a cubelist, then we should update type hints and make variable name more generic
| base_time_coord = base.coord(time_coord) | ||
| other_time_coord = other.coord(time_coord) | ||
| logger.debug("Base: %s\nOther: %s", base_time_coord, other_time_coord) | ||
| if time_coord == "hour": |
There was a problem hiding this comment.
why would we not not use datetime for all comparisons (i.e. why do we have a separate one for if the time_coord is 'hour'). I guess I don't know what is meant by 'non-absolute units, such as hour'.
There was a problem hiding this comment.
So this was in the original extract_common_time_points function and not something I added. I am unsure of the reasoning here, but I can test this to see if it's actually needed.
| def extract_common_time_points(cubes): | ||
| """Return cubes with common time points.""" |
There was a problem hiding this comment.
typehints, as public function? Also parameters, and returns in docstring. As this gets pulled through into our documentation pages.
|
|
||
| return regridded_cubes | ||
|
|
||
| # # Ensure matching times in fld cube and point_cube |
There was a problem hiding this comment.
why is there a load of commented code?
| if not np.array_equal(reference_frts, cube_frts): | ||
| raise ValueError( | ||
| "Cubes do not share the same forecast_reference_time values." | ||
| ) | ||
|
|
||
| except iris.exceptions.CoordinateNotFoundError as err: | ||
| raise ValueError( | ||
| "All cubes must have a forecast_reference_time coordinate." | ||
| ) from err |
There was a problem hiding this comment.
both of these raises are not tested, according to the coverage report https://tmpweb.net/KFAQY4uOKECa/z_f5ac9763053bfad5_misc_py#t487
Co-authored-by: James Warner <62252918+jwarner8@users.noreply.github.com>
Co-authored-by: James Warner <62252918+jwarner8@users.noreply.github.com>
Co-authored-by: James Warner <62252918+jwarner8@users.noreply.github.com>
Co-authored-by: James Warner <62252918+jwarner8@users.noreply.github.com>
Co-authored-by: James Warner <62252918+jwarner8@users.noreply.github.com>
This branch has not been deployed
3 participants
fixes #2447
Included functionality to extract common time points for cubes with multiple FRTs
Moved this step outside of functions and as a step in recipes e.g rather than embedded.
For functions that used this, included a checking function to ensure correct time coordinates between cubes.
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.