Skip to content

Conversation

@BernardClaxton
Copy link
Contributor

@BernardClaxton BernardClaxton commented Nov 27, 2025

Fixes #1759

Contribution checklist

Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.

  • Documentation has been updated to reflect change.
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Ensured the pull request title is descriptive.
  • Conda lock files have been updated if dependencies have changed.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

@BernardClaxton BernardClaxton self-assigned this Nov 27, 2025
@BernardClaxton BernardClaxton added the enhancement New feature or request label Nov 27, 2025
@Sylviabohnenstengel
Copy link
Member

for info if the upgrade of you branch with main causes problems then I have created a branch of your branch and merged main in now including the addition of plotting transverse mercator. So you should be able to plot the radar data.
Branch: #1759_import_nimrod_merge_main

@Sylviabohnenstengel
Copy link
Member

Sylviabohnenstengel commented Jan 20, 2026

#https://github.com/MetOffice/CSET/tree/1759_import_nimrod_merge_main adds nimrod radar switch into rose gui and enables nimrod switch in flow.cylc for the workflow.

@jfrost-mo jfrost-mo changed the title supporting UK radar data Support UK radar data Jan 23, 2026
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/CSET/cset_workflow/app/fetch_fcst/bin/fetch_data.py is the main file for this change, as the stuff in fetch_nimrod is largely unused.

What is our plan with this? Are we going to try and reuse the RES capability in cube_utils? Or do we not really need it given the existing capabilities of CSET?

The recipe and such looks broadly sensible, though I think we can get away with hard-coding some more for the initial simple recipe.

Comment on lines +107 to +112
[[FETCH_NIMROD]]
script = rose task-run -v --app-key=fetch_nimrod
execution time limit = PT1H
[[[environment]]]
ANALYSIS_LENGTH = {{ANALYSIS_LENGTH}}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make fetch_nimrod inherit from the FETCH_DATA family. Then we don't need a new family, don't need to add it to the graph above, and don't need a new dummy task.

See how the surface obs do it on line ~180.

Comment on lines +29 to +32
# for model, field, method in itertools.product(
for model, field in itertools.product(
models, conf.SURFACE_FIELDS, conf.SPATIAL_SURFACE_FIELD_METHOD
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think need to loop here, as the field will remain constant for radar, and we don't need to worry about different models either.

So I think this can become something like:

if conf.NIMROD_FIELDS_COMP_HOUR: # Note: You might want to use a different setting.
    yield RawRecipe(
        recipe="generic_surface_spatial_plot_sequence_radar.yaml",
        variables={ "VARNAME": conf.NIMROD_FIELDS_COMP_HOUR },
        aggregation=False,
    )

operator: constraints.combine_constraints
varname_constraint:
operator: constraints.generate_var_constraint
varname: $VARNAME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we get away with hardcoding VARNAME? Or do we need it to support different resolution data?

Comment on lines +16 to +29
# cell_methods_constraint:
# operator: constraints.generate_cell_methods_constraint
# cell_methods: []
# varname: $VARNAME
# level_constraint:
# operator: constraints.generate_level_constraint
# coordinate: "pressure"
# levels: []
# subarea_type: $SUBAREA_TYPE
# subarea_extent: $SUBAREA_EXTENT

# - operator: collapse.collapse
# coordinate: [time]
# method: $METHOD
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# cell_methods_constraint:
# operator: constraints.generate_cell_methods_constraint
# cell_methods: []
# varname: $VARNAME
# level_constraint:
# operator: constraints.generate_level_constraint
# coordinate: "pressure"
# levels: []
# subarea_type: $SUBAREA_TYPE
# subarea_extent: $SUBAREA_EXTENT
# - operator: collapse.collapse
# coordinate: [time]
# method: $METHOD

Comment on lines +2 to +4
#title: $MODEL_NAME $VARNAME $METHOD
#description: Extracts and plots the $METHOD of $VARNAME from all times in $MODEL_NAME.
title: $VARNAME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#title: $MODEL_NAME $VARNAME $METHOD
#description: Extracts and plots the $METHOD of $VARNAME from all times in $MODEL_NAME.
title: $VARNAME
title: Radar plot of $VARNAME

Add a little bit more to the title so it is useful.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused. Are we implementing the fetching of radar in fetch_fcst, or separately in fetch_nimrod?

"""Write the dictionary to retrieve Nimrod obs."""
files = {
"rainaccum_comp_hour": {
"basedir": "/data/users/radar/UKnimrod",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these paths too Met Office specific? They might want to be pushed into a per-site file and then moved to the CSET restricted files repository.

print(" Copying Nimrod files to directory: ", cycle_nimrod_dir)
for i in filepaths:
print(" Grabbing Nimrod file: ", i)
shutil.copy(i, cycle_nimrod_dir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since they are already on disk, can we just symlink to them to avoid copying data about?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is unused. Is it only a reference for implementing src/CSET/cset_workflow/app/fetch_nimrod/bin/fetch-nimrod.py? (I presume it comes from the RES?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so this file just calls the fetch_nimrod function in src/CSET/cset_workflow/app/fetch_fcst/bin/fetch_data.py, and none of the other code is currently used for anything.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2026

Coverage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Importing Nimrod radar observations

4 participants