Updating wind downscaling code to be more descriptive by mo-AliceLake · Pull Request #2315 · metoppv/improver

mo-AliceLake · 2026-02-26T12:30:33Z

This issue represents step 1 of a larger piece of work to enable skilful wind downscaling in IMPROVER. The current wind‑downscaling code mixes height correction (HC) and roughness correction (RC). My understanding is that earlier investigations suggest that HC alone adds skill, but the combined HC+RC approach does not (at least over the UK domain). The long‑term aim (step 2) is to separate these components cleanly, but to be able to do that, the existing code needs to be understood, tidied, and documented without altering the science. This issue covers that preparatory clean‑up work.

Testing:

Ran tests and they passed OK
Added new tests for the new feature(s) - N/A

CLA

If a new developer, signed up to CLA

… and function names, etc

SamGriffithsMO

I'm not sure of the exact context of this work, but if we want the code to run outside of the Improver suite on the HPC, you may wish to also update cli/wind_dowscaling.py. Ideally it would just initialise plugins and call them, making calling via the API easier.

However if you do update the CLI to align with these changes (naming conventions) you will break the CLI and API. Are the full scope of your changes going break these anyway? And therefore, should you target this at a feature branch? (The Improver science team can hopefully set expectations here, as they have better sight on all the uses of these plugins).

If you don't need to break CLI or API with your changes, I would advise not breaking them (i.e. revert changes to init and process call signatures)

SamGriffithsMO · 2026-03-10T10:40:02Z

+            except CoordinateNotFoundError:
+                ok_pairs.append(False)


Maybe not for this PR, but it would be helpful to give a hint as to what/why this returned false

I need someone to teach me how to do logging in IMPROVER - note to remind me to ask 🙂

mo-AliceLake · 2026-03-10T11:09:59Z

I'm not sure of the exact context of this work, but if we want the code to run outside of the Improver suite on the HPC, you may wish to also update cli/wind_dowscaling.py. Ideally it would just initialise plugins and call them, making calling via the API easier.

However if you do update the CLI to align with these changes (naming conventions) you will break the CLI and API. Are the full scope of your changes going break these anyway? And therefore, should you target this at a feature branch? (The Improver science team can hopefully set expectations here, as they have better sight on all the uses of these plugins).

If you don't need to break CLI or API with your changes, I would advise not breaking them (i.e. revert changes to init and process call signatures)

I don't know about the IMPROVER cli and API - need to learn! - but unfortunately yes this work will definitely be a breaking change as we need to split out HC and RC (see e.g. work-in-progress next step #2324)

Will ask about feature branch 🙂

mo-AliceLake · 2026-03-10T15:02:43Z

Thanks to @SamGriffithsMO's pointers:

I've learnt about the IMPROVER api (didn't need updating) and cli (have now updated accordingly).
I have made a feature branch feature_update_wind_downscaling which I am now pointing everything towards. Can do proper release notes etc when that feature branch is eventually merged into master.

And in the meantime, unit tests have started failing, but that's not to do with these changes - we think to do with something being updated in the environment.

SamGriffithsMO

I'm happy with this (less the failing tests). Since it's not pointing at main I'll approve anyway, and pass this on to Ben A(?) who might be better able to advise on a CI fix

* Pinning proj to 9.7.1 * Adding myself to contributors list * Fix numpy-update test failures * Reverting float() change

bayliffe

Thanks for tackling this Alice, we've all been far too scared. A couple more comments here to go with those below.

We have generally removed the type definitions from the doc-string text, instead relying upon the type-hinting in the function interfaces to specify this; have a look across a few other plugins. I don't think this needs to be a hard rule, so if you find it clearer to include the types in the text as well, that's fine.
Do you think you will tackle rewriting / improving the unit tests as part of the broader work? I'll flag now that if they are to undergo a serious rewrite it would be good to move to using pytest, which we've not done wholesale but are tackling piecemeal as we happen to be working on older tests.

bayliffe · 2026-03-16T15:49:57Z

 Aaron Hopkinson <aaron.hopkinson@metoffice.gov.uk> <arh89@users.noreply.github.com>
+Alice Lake <alice.lake@metoffice.gov.uk> <69578135+mo-AliceLake@users.noreply.github.com>
 Andrew Creswick <andrew.creswick@metoffice.gov.uk> AndrewCreswick <andrew.creswick@metoffice.gov.uk>
+Alice Lake <alice.lake@metoffice.gov.uk> <69578135+mo-AliceLake@users.noreply.github.com>


Do we need the duplicate here?

bayliffe · 2026-03-16T15:56:01Z

                )
            )
        wind_speed = single_level
+


As Sam mentioned, this CLI is doing too much. Ideally, though not universally, the CLIs should gather arguments and call a plugin and that's about it. There is a future in which the CLIs never get called, therefore any processing in them will be lost. For that reason we have been either folding the functionality into the plugins themselves, or creating wrappers that basically pull the CLI functionality into more standard code. For example the spot-extract CLI used to do many things, but the SpotManipulation plugin was created to bundle that up as a new plugin.

https://github.com/metoppv/improver/blob/master/improver/cli/spot_extract.py

This doesn't have to be done here and now, I would suggest writing up a follow up ticket to capture the requirement.

bayliffe · 2026-03-16T16:06:14Z

Vosper and Clark are mentioned in this code, but the paper references are not included formally as far as I can see. We can include a References: section in doc-strings to capture this as per the style guide. There are lots of examples around, e.g. reliability calibration. Somewhere, I leave it to you to decide where, the references relevant to wind downscaling should be formally listed.

bayliffe · 2026-03-16T16:15:41Z


-    This holds functions to calculate the roughness and height
+    This class holds functions to calculate the roughness and height
    corrections given the ancil files:


I wouldn't class the wind_field as an ancillary.

...given the ancil and forecast files: ?

bayliffe · 2026-03-16T16:20:36Z

+      - silhouette_roughness (ndarray): Dimensionless measure of sub-grid
+        terrain.
+      - roughness_length_z0 (ndarray): Vegetative roughness length.
+      - orog_pp (ndarray): Orography to correct to.


orog_pp is our target_orography to which we are adjusting the winds. As such I would find target_orography a more intuitive name. You might not of course, in which case, fair enough.

bayliffe · 2026-03-16T16:21:33Z

+            res_pp (float):
+                Grid cell resolution of the post-processing grid.
+            res_model (float):
+                Grid cell resolution of the model grid.


Native horizontal model-grid resolution (m).

To match the more descriptive definition elsewhere in this file. A similar improvement might be made for res_pp above so that some units are provided.

bayliffe · 2026-03-16T16:38:03Z

+        self.hc_mask[self.orog_pp == RMDI] = False
+        self.hc_mask[self.orog_model == RMDI] = False
+        self.hc_mask[np.isnan(self.orog_pp)] = False
+        self.hc_mask[np.isnan(self.orog_model)] = False


As a single statement should you prefer it, though possibly clearer line-by-line.

Suggested change

self.hc_mask[self.orog_pp == RMDI] = False

self.hc_mask[self.orog_model == RMDI] = False

self.hc_mask[np.isnan(self.orog_pp)] = False

self.hc_mask[np.isnan(self.orog_model)] = False

self.hc_mask[

np.equal(self.orog_pp, RMDI) | np.equal(self.orog_model, RMDI) |

np.isnan(self.orog_pp) | np.isnan(self.orog_model) |

] = False

bayliffe · 2026-03-16T17:02:02Z

+        # Pairwise x/y grid compatibility check across all ancils
+        ok_pairs: list[bool] = []
+        for a, b in itertools.permutations(required, 2):
+            try:
+                same_y = a.coord(axis="y") == b.coord(axis="y")
+                same_x = a.coord(axis="x") == b.coord(axis="x")
+                ok_pairs.append(bool(same_x & same_y))
+            except CoordinateNotFoundError:
+                ok_pairs.append(False)


You could make use of the spatial_coords_match functionality in utilities/cube_checker.py rather than this being repeated here. It would remove all of the stripping of extra coordinates etc above which should shorten this quite a bit. You may wish to retain this for the units comparison and to invoke the suggested function.

bayliffe · 2026-03-16T17:07:58Z


-        Check if wind and ancillary files are on the same grid and if
-        they have the same ordering.
+    def check_wind_ancil(self, xwp: int, ywp: int) -> None:


This could be replaced with a call to enforce_coordinate_ordering in utilities/cube_manipulation.py. Instead of simply checking the order matches and failing otherwise the code can simply enforce that the orders match and so allow it to continue. Deletes some more code from this file as well.

mo-AliceLake added 6 commits February 26, 2026 11:30

Updating wind downscaling code to use more descriptive variable names…

2325556

… and function names, etc

Adding name to CLA

51746cf

Adding myself to .mailmap

3f6e5eb

Adding myself to .mailmap

b4c87a3

Adding myself to .mailmap

15d46d8

Adding myself to .mailmap

d6e4f82

mo-AliceLake marked this pull request as draft March 3, 2026 13:34

mo-AliceLake added 3 commits March 3, 2026 13:36

Trying without space

d2ba8a2

Fixing unit tests

1b26277

Merging master

cabd6c1

mo-AliceLake marked this pull request as ready for review March 5, 2026 15:47

mo-AliceLake added 2 commits March 6, 2026 09:07

Updating error log message

4a62b37

Attempting to fix .mailmap by using anon github email

f7fedbe

mo-AliceLake mentioned this pull request Mar 9, 2026

Enable independent HC and RC in wind downscaling #2324

Draft

2 tasks

SamGriffithsMO self-requested a review March 10, 2026 08:58

SamGriffithsMO reviewed Mar 10, 2026

View reviewed changes

mo-AliceLake added 2 commits March 10, 2026 13:20

Changing u* -> u_star for clarity

e9b756a

Changing missing spatial coordinates to match old behaviour

b5645d0

mo-AliceLake changed the base branch from master to feature_update_wind_downscaling March 10, 2026 14:11

Update cli

9d4e85d

SamGriffithsMO approved these changes Mar 10, 2026

View reviewed changes

mo-AliceLake and others added 2 commits March 11, 2026 11:27

Pin proj and fix numpy-update-related test failures (#2326)

9bbbf7a

* Pinning proj to 9.7.1 * Adding myself to contributors list * Fix numpy-update test failures * Reverting float() change

Merging in master

f057682

bayliffe requested changes Mar 16, 2026

View reviewed changes

Conversation

mo-AliceLake commented Feb 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamGriffithsMO left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mo-AliceLake commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mo-AliceLake commented Mar 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SamGriffithsMO left a comment

Choose a reason for hiding this comment

Uh oh!

bayliffe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mo-AliceLake commented Feb 26, 2026 •

edited

Loading

mo-AliceLake commented Mar 10, 2026 •

edited

Loading

mo-AliceLake commented Mar 10, 2026 •

edited

Loading