Skip to content

Implement calculate wind_speed from components#2241

Merged
ukmo-huw-lewis merged 1 commit into
mainfrom
2239-compute-wind-speed-from-components
Jun 30, 2026
Merged

Implement calculate wind_speed from components#2241
ukmo-huw-lewis merged 1 commit into
mainfrom
2239-compute-wind-speed-from-components

Conversation

@ukmo-huw-lewis

@ukmo-huw-lewis ukmo-huw-lewis commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #2239.

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.
  • Ensure rose-suite.conf.example has been updated if new diagnostic added.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

@ukmo-huw-lewis ukmo-huw-lewis linked an issue Jun 29, 2026 that may be closed by this pull request
@ukmo-huw-lewis

Copy link
Copy Markdown
Contributor Author

Summary:

  • Updated varname constraint to accept more fields
  • Ensure fix_um_winds works as required

Mark ready for review.

@github-actions

Copy link
Copy Markdown
Contributor

Coverage

@jfrost-mo

Copy link
Copy Markdown
Member

See also #2119

@mo-nmakryg mo-nmakryg left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comments:

  1. From this branch, the total wind speed is included and appears to be working as expected, with no obvious impact on any other variables.
  2. However, the land and sea masks are still crashing, so I had to force the run to be marked as successful in order to obtain the results. I expect this issue will be resolved separately as part of other fixes.
  3. I haven’t worked a lot with the vectors as suggested, and some fixes I guess will come from See also #2119 (per James F. comment)
  4. I haven’t checked on the multivariable plots for now as Claudio is working on this functionality in parallel, and also the vectors may be a good option for layering, so we may need these first.

If developers are happy with these asterisks of remaining task I think the branch is ready to be merged.

PS. For awareness I was removing the obs by turning them off but just doing this was giving me

PluginError: Error in plugin cylc.pre_configure.rose
ConfigProcessError: template variables=POINT_OBS_WMO_BLOCK_STTN_NUMBERS=: Invalid template variable:
Must be a valid Python or Jinja2 literal (note strings "must be quoted").

So I had to vi rose-suit.conf in order to comment this out. Not sure if I was doing something wrong through the gui or there is a bug.

@ukmo-huw-lewis

Copy link
Copy Markdown
Contributor Author

Thanks for review comments.

Proposing to merge as-is.

a) Think pre-existing issues in Nefeli's workflow (e.g. land/sea mask) not impacted by this change. Other gui errors related to use of rose-suite.conf updated to run with OBS branch-in-progress to test this more limited changeset.

b) Thanks to James for flagging #2119 - note here the aim is to update the default action when requesting "wind_speed_at_10m" (i.e. enabling existing fix_um_winds callback) rather than requiring this to be initiated via specific operator - which is also useful functionality. This enables wind_speed_at_10m (for example) to be requested as one of 'generic' variables in list without needing to call the operator via recipe. Can review if fix_um_winds might be better replaced by call to the specific operator in code when that PR is on main perhaps.

c) This PR re-highlights that vector_plot is not, to best of my knowledge, well covered by unit tests (see plot.py coverage report) and don't think it is 'plumbed in' to the workflow. Following #2238, would be route to retiring vector_plot but extend functionality of plot_spatial to also accept vector component inputs - this provides ability to overplot a vector over any base field, not just the magnitude (e.g. plot current vectors on top of SST base).

@ukmo-huw-lewis ukmo-huw-lewis merged commit 99f9716 into main Jun 30, 2026
7 checks passed
@ukmo-huw-lewis ukmo-huw-lewis deleted the 2239-compute-wind-speed-from-components branch June 30, 2026 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Compute wind speed from components

3 participants