Add Cardington recipes#2128
Conversation
Total coverage: 91% (HTML report)Name Stmts Miss Branch BrPart Cover --------------------------------------------------------------------------------------------------- src/CSET/__init__.py 93 2 12 0 98% src/CSET/_common.py 149 0 52 0 100% src/CSET/cset_workflow/app/fetch_fcst/bin/fetch_data.py 115 27 24 0 79% src/CSET/cset_workflow/app/finish_website/bin/finish_website.py 70 0 4 0 100% 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 47 0 16 0 100% src/CSET/graph.py 43 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 229 4 62 4 97% src/CSET/operators/_stash_to_lfric.py 3 0 0 0 100% src/CSET/operators/_utils.py 183 8 74 6 95% src/CSET/operators/ageofair.py 141 7 64 5 94% src/CSET/operators/aggregate.py 76 1 22 1 98% src/CSET/operators/aviation.py 60 0 18 0 100% src/CSET/operators/collapse.py 154 12 72 5 91% src/CSET/operators/constraints.py 111 7 48 2 93% src/CSET/operators/convection.py 37 4 10 2 87% src/CSET/operators/ensembles.py 27 0 14 0 100% src/CSET/operators/feature.py 41 0 10 0 100% src/CSET/operators/filters.py 66 2 30 0 98% src/CSET/operators/fluxes.py 41 0 10 0 100% src/CSET/operators/humidity.py 139 0 56 0 100% src/CSET/operators/imageprocessing.py 56 0 16 0 100% src/CSET/operators/mesoscale.py 17 0 2 0 100% src/CSET/operators/misc.py 158 1 66 4 98% src/CSET/operators/plot.py 1011 180 346 67 78% src/CSET/operators/power_spectrum.py 97 3 30 3 95% src/CSET/operators/precipitation.py 176 34 84 4 80% src/CSET/operators/pressure.py 41 0 12 0 100% src/CSET/operators/read.py 409 38 178 14 89% src/CSET/operators/regrid.py 122 18 64 1 83% src/CSET/operators/scoreswrappers.py 47 6 12 3 85% src/CSET/operators/temperature.py 121 0 32 0 100% src/CSET/operators/transect.py 62 0 24 0 100% src/CSET/operators/wind.py 45 3 10 2 91% src/CSET/operators/write.py 15 0 6 0 100% src/CSET/recipes/__init__.py 101 0 28 0 100% --------------------------------------------------------------------------------------------------- TOTAL 4472 357 1566 123 91% |
ukmo-huw-lewis
left a comment
ukmo-huw-lewis
left a comment
There was a problem hiding this comment.
In my view, would be beneficial to adopt some more generalised recipe approach here to avoid 'explosion' of recipes for different plot types (line series, histogram, spectra, etc).
Some templates potentially exist via #2084.
Exceptions likely include where conversions required between observed variable and modelled variable (e.g. LH, SH). Happy to 'workshop' approach to recipes in this instance.
Propose to provide example cset bake example pointing to sample data, and can work up some aspects of generalisation.
e.g. these recipes should support potentially different obs inputs and multiple different models. This is achievable, I think, by relying on $INPUT_PATHS and $MODEL_NAMES as inputs. Working through with some example data will help illustrate and revise recipes.
Happy to support your development to help progress.
Simon Osborne (mo-sro)
changed the title
ukmo-huw-lewis
left a comment
ukmo-huw-lewis
left a comment
There was a problem hiding this comment.
Please update Contribution checklist.
Please update branch to merge with main via PR.
Please provide example usage (e.g./i.e. "cset bake command) to support independent review of this change.
…cardington_recipes
ukmo-huw-lewis
left a comment
ukmo-huw-lewis
left a comment
There was a problem hiding this comment.
Have now reviewed and left many detailed comments to consider and please address. In general, looks in decent shape, so more about fine-tuning I think.
In summary:
- Number of stylistic aspects relating to choice of variable naming within recipes to address
- Consider if possible to adopt existing
generate_maskandapply_maskoperator in recipes rather than introducemask_fill_valuesand related PR etc. - Aim for more consistent ordering of operators across all recipes
To consider:
- This PR likely still balancing act between a) as few recipes as possible to support multiple different potential variables and b) very clear 1-recipe-per-variable approach.
- I don't think I mind which approach is adopted, but note currently we lose some of the clarity by adopting general title/varnames in recipes through env variables, and requires users to have to control and get right with more inputs, but equally not benefitting from being able to rationalise number of recipes by doing so.
- If pushed, I might advocate continuing with 1-recipe-per-variable (might make some aspects of workflow implementation easier, and less bespoke to Cardington use-case), and so might remove some of the generalisations currently in recipes.
- Please consider some of above in context of any requirements to further extent coverage of variables.
- Please also consider how recipe set might be extended to capture additional output plot types (histogram, scatter plot etc), and whether some decisions on approach here might make that simpler.
Happy / keen to discuss and support further.
| @@ -0,0 +1,63 @@ | |||
| category: Time series of latent heat flux at Cardington single point | |||
There was a problem hiding this comment.
| category: Time series of latent heat flux at Cardington single point | |
| category: Time series at Cardington |
Suggest category should be something relatively general that lots of outputs might sit in (e.g. generic_spatial_plots outputs all have category "Surface Spatial Plot".
The main aim of category is to offer some organisation/cataloguing of output plots.
Suggest you have same category wording for all Cardington time series recipes.
| @@ -0,0 +1,63 @@ | |||
| category: Time series of latent heat flux at Cardington single point | |||
| title: "Time series of $PLOT_VARNAME at lat_pt:52.10438, lon_pt:-0.42286 specific to Cardington gridpoint." | |||
There was a problem hiding this comment.
| title: "Time series of $PLOT_VARNAME at lat_pt:52.10438, lon_pt:-0.42286 specific to Cardington gridpoint." | |
| title: "Time series of latent heat flux at Cardington [lat:52.10438, lon:-0.42286]" |
Suggesting:
a) given current approach is a different recipe per variable type, be explicit on the variable name in recipe title. If we end up with more generic recipe template for all/many Cardington obs types, then suggest that time to introduce a $PLOT_VARNAME input. Otherwise sense these recipes require user to track lots of flavours of 'VARNAME'.
b) aim for shorter title as will improve plot readability / dimensions etc. Alternative suggested.
Implement similar changes across proposed recipes.
There was a problem hiding this comment.
Similarly, remove $PLOT_VARNAME from the description line below.
Alternative strategy would be to consolidate more of these recipes into generic Cardington time series recipe, but under impression that is not desirable (e.g. due to need for variable-specific processing).
| - operator: read.read_cubes | ||
| file_paths: $INPUT_PATHS | ||
| model_names: | ||
| - Cardington 30min |
There was a problem hiding this comment.
| - Cardington 30min | |
| - OBS (30min) |
Not wholly wedded to this, but for e.g. external publication, keeping legend titles as simple as possible may have value, and not obvious what "Cardington" is in plot label here.
Cardington as location is highlighted in the simpler plot title.
Consider implementing this approach across recipes.
| model_names: | ||
| - Cardington 30min | ||
| - $MODEL_NAME | ||
| constraint: ['$VARNAME', '$UM_VARNAME'] |
There was a problem hiding this comment.
| constraint: ['$VARNAME', '$UM_VARNAME'] | |
| constraint: ['$OBS_VARNAME', '$VARNAME'] |
Strongly suggest we need to keep "VARNAME" as consistent with general CSET use of "VARNAME" (typically standard_names, model output etc), and also that we should remove explicit referneces to 'UM' here.
Therefore instead, introduce $OBS_VARNAME to cater for any obs-specific naming conventions.
There was a problem hiding this comment.
Would need to make equivalent alternations further down in recipe where currently $VARNAME and $UM_VARNAME referenced.
| attribute: STASH | ||
|
|
||
| - operator: fluxes.latent_heat_units | ||
|
|
There was a problem hiding this comment.
Consider adding in call to filter fill_values (see example comment below) here for consistency with SH flux recipe.
| - date | ||
| - date_created | ||
|
|
||
| - operator: misc.convert_visibility_to_km |
There was a problem hiding this comment.
check if this required, or if
read._convert_cube_units_callback() in read.py doesn't already cover this? And if not, why not?
That function includes test of varnames for "visibility" and then converts units to km.
| category: Time series of relative humidity at Cardington single point | ||
| title: Time series of 'relative humidity' at lat_pt:52.10438, lon_pt:-0.42286 specific to Cardington gridpoint. | ||
| description: Plots a time series of the relative humidity at a selected Cardington gridpoint. | ||
| category: Time series of variables at Cardington single point |
There was a problem hiding this comment.
Has the previous recipe covering relative humidity been lost from this changeset?
Or presumably already in the supported code?
If so, are any amendments required in pre-existing Cardington recipes for consistent style etc.
| - Cardington 05min | ||
| - Cardington 30min | ||
| - $MODEL_NAME | ||
| constraint: ['$VARNAME', '$UM_U_VARNAME', '$UM_V_VARNAME'] |
There was a problem hiding this comment.
Reminder of earlier comments on adopting more general varname naming conventions.
Here I would propose something like:
['$OBS_VARNAME', '$U_VARNAME', '$V_VARNAME']
On other hand, more flexible support for "wind_speed_at_10m" might now allow for simpler case where can just call with single varname and the code will ensure components are read in successfully.
| value: $HEIGHT | ||
|
|
||
| second: | ||
| # --- UM: calculate vector wind from both components --- |
There was a problem hiding this comment.
I think this is now correctly handled in code, at least for case of wind_speed_at_10m.
It was not working correctly when this development was started.
Can we review if working with "wind_speed_at_10m" recipe will now work with less intervention?
Is there requirement to support wind calculations at different heights? Looks to me like this recipe is currently working with near-surface (10m) winds from model only? So anticipating that varname better supported in code now.
| operator: constraints.generate_attribute_constraint | ||
| attribute: STASH | ||
|
|
||
| # PLOT_VARNAME is used to constrain to either speed or direction |
There was a problem hiding this comment.
OK, so this recipe aiming to support both speed and direction.
Maybe another example where review of benefits/disbenefits of even more generalisation (only few bespoke variable recipes, but most can run from generic cardington_single_point_time_series.yaml recipe, and then some more variable-specific recipes).
3 participants
Contribution checklist
Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.
rose-suite.conf.examplehas been updated if new diagnostic added.