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-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 · 2026-04-29T12:12:30Z

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.

Thanks so much for such a detailed+helpful review! 🙂 Really useful for getting more familiar with the IMPROVER codebase.

I've made pretty much all the suggested changes (including tidying up the type hints to align with the rest of the repo) and have also reworked the docstrings and variable names, so hopefully these are now much clearer.

There are a couple of things I've deliberately left for follow-up work to avoid this one becoming too tangled:
Refactoring the CLI to remove the processing (ticket here: https://github.com/metoppv/mo-blue-team/issues/1141)
Updating to newer IMPROVER utilities, because I'd like to tackle this once I have stronger unit test coverage to make sure I don't accidentally change any behaviour...

Tests: yes, I'm planning to rewrite these (have started having a go on a separate branch) but hadn't yet switched to pytest, so thanks for flagging - I'll have a go. 🙂

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

Merged

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

mo-AliceLake added 4 commits April 28, 2026 16:10

Making variable names clearer

adace4c

Removing duplicate entry

ebe6e91

Updating variable names to be more intuitive

ccfad17

Improving variable names and docstrings

b1db175

Updating variable names in unit tests to match code

b570e4a

mo-AliceLake merged commit 5709753 into feature_update_wind_downscaling Apr 30, 2026
7 checks passed

mo-AliceLake deleted the alice-update-winddownscaling branch April 30, 2026 09:33

Uh oh!

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!

SamGriffithsMO Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

mo-AliceLake Mar 10, 2026

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bayliffe Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

bayliffe Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

mo-AliceLake commented Apr 29, 2026

Uh oh!

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