Implement new wind orographic correction method by mo-AliceLake · Pull Request #2447 · metoppv/improver

mo-AliceLake · 2026-09-02T16:00:04Z

Addresses https://github.com/metoppv/mo-blue-team/issues/1277

Description

Implements the new wind orographic correction method within the IMPROVER repository, replacing the legacy wind downscaling approach.

This PR:

Adds the new wind orographic correction implementation following the recommendations from https://github.com/metoppv/mo-blue-team/issues/1270
Removes legacy wind downscaling functionality and associated references
Adds documentation, inline comments, and docstrings where appropriate
Updates unit tests for the new functionality
Updates KGO/acceptance tests to validate the new behaviour - test data in Wind orographic correction test data improver_test_data#148

Testing

Ran tests and they passed OK
Added new tests for the new feature(s)

Test coverage

Unit tests added for the new wind orographic correction method
KGO/acceptance tests updated to reflect the new implementation

* Updating wind downscaling code to use more descriptive variable names and function names, etc * Adding name to CLA * Adding myself to .mailmap * Adding myself to .mailmap * Adding myself to .mailmap * Adding myself to .mailmap * Trying without space * Fixing unit tests * Updating error log message * Attempting to fix .mailmap by using anon github email * Changing u* -> u_star for clarity * Changing missing spatial coordinates to match old behaviour * Update cli * Pin proj and fix numpy-update-related test failures (#2326) * Pinning proj to 9.7.1 * Adding myself to contributors list * Fix numpy-update test failures * Reverting float() change * Making variable names clearer * Removing duplicate entry * Updating variable names to be more intuitive * Improving variable names and docstrings * Updating variable names in unit tests to match code

* Updating wind downscaling code to use more descriptive variable names and function names, etc * Adding name to CLA * Adding myself to .mailmap * Adding myself to .mailmap * Adding myself to .mailmap * Adding myself to .mailmap * Trying without space * Fixing unit tests * Updating error log message * Attempting to fix .mailmap by using anon github email * Initial commit to split out HC + RC * Renaming tests to make them more descriptive * Working on unit tests * Changing u* -> u_star for clarity * Changing missing spatial coordinates to match old behaviour * Update cli * Changing name in __init__.py * Making RC fail when no roughness length supplied * Updating tests * Updating docstrings of unit tests to match test names * Making variable names clearer * Removing duplicate entry * Updating variable names to be more intuitive * Improving variable names and docstrings * Updating variable names in unit tests to match code * Fixing to work with surface data * Addressing PR review comments * Further tidying * Fixing acceptance tests * Updating unit test to fix HC + RC behaviour

mo-jbeaver

Overall, the function and tests are very comprehensive. The majority of the comments I have added are about the docstrings or adding comments for clarity. Unit test ran successfully, acceptance tests failed due to checksum mismatches. There was also a list of failures from when I ran srun_tests, that maybe a result of the branch being out of date.

mo-AliceLake · 2026-10-05T14:12:30Z

Thank you for such a careful and detailed review @mo-jbeaver 😊 It may take me a while to work through these, but only because I want to go through each comment properly!

mo-AliceLake · 2026-10-06T15:43:48Z

Thanks you so much for such a comprehensive review. 😊

The acceptance tests are passing for me locally, so I wonder if the checksum mismatches could be due to the test-data version being used? Are you running against the updated test data in the linked PR? If not, I'd expect checksum failures because the accepted outputs have changed (I'm a bit out of my depth here with the acceptance tests though, so if that's not the cause, I might need to crowd-source some debugging!)

metoppv/improver_test_data#148

Yes, I think the srun_tests failures may also be related if the branch is a little out of date.

mo-jbeaver · 2026-10-07T14:02:35Z

Thanks for the update @mo-AliceLake , I'm happy with the code changes made. Both the unit and acceptance tests passed but the srun_tests (which runs all unit and acceptance tests to look for conflicts) failed.
I had a look at the error outputs and there seems to be three types of issues.

Files not being found - There a number of files that these tests cannot find, these are for other functions (quantile_mapping) and (stochastic_noise). This is likely to be caused by your improver_test_data branch not being up-to-date with the latest changes (equivalent to merging master into your branch on improver).
The function improver_tests/acceptance/test_compare.py::test_same uses the kgo's wind_downscaling/basic/highres_orog.nc and wind_downscaling/veg/highres_orog.nc as part of it's check. These, however, don't exist anymore in your PR. So likely this function will need to be update in some respect.
Finally, there was also a checksum issue. The error output included the line: Right contains 34 more items, first extra item: 'b63df9d5699eead93e001ce9d3ea7fe342bc828175daeba55d77711fb5cd63fb wind-orographic-correction/no_realizations/kgo_target_height_levels.nc'. From this, it seems that while the checksums were updates for the new kgo's and the previous ones deleted, the checksums for these now deleted kgo were not also deleted causing a conflict.

Hopefully, that should explain the errors I was getting but again happy with the code changes made.

mo-AliceLake · 2026-10-08T10:03:33Z

Thanks so much for your help @mo-jbeaver 😊 Your explanations were enough to help me work through everything in the end!

I've now merged master into both branches (here and the improver_test_data PR), removed the stale wind_downscaling checksums, and updated test_compare.py::test_same to use the updated wind-orographic-correction orography files.

The only remaining checksum difference to me seems to be visibility-combine-cloud-base/spot/kgo.nc. That appears to be because its test-data branch mobt_1291_visibility_cloud_base_update hasn't yet been merged into improver_test_data master yet (so it's unrelated to this PR) - I've asked if that can be merged. - Edit: This is now merged. 🙂

mo-jbeaver

Happy with all the updates made and the tests all passed successfully.

bayliffe

I am by no means finished with this review, but submitting to ensure the comments thus far are not lost. Please ignore until I am finished.

bayliffe · 2026-10-14T14:27:55Z

+    if cube_heights.size == 1:
+        raise ValueError(
+            "Cannot interpolate wind_profile_cube to target_height_levels "
+            "when only one source height is available."
+        )


I wondered why this check lived here rather than in the process method when the data is first ingested and then I noticed that this is a standalone function, as are many of these rather than methods on a plugin. Is there a reason for these to be standalone functions rather than components of a broader plugin as is more typical within IMPROVER?

"Is there a reason for these to be standalone functions rather than components of a broader plugin as is more typical within IMPROVER?" - No reason, I've just never really coded with plugins. I'll try and learn a bit more then refactor.

bayliffe

These are the rest of my comments for this pass. Thanks for all of this work.

bayliffe · 2026-10-15T12:36:59Z

+        max_friction_velocity=5.0,
+    )
+
+    assert np.all(result > 0.0)


What in this test setup would cause the values to become negative and thus demonstrates that somehow this code ensures only positive values are returned? I cannot see it.

mo-AliceLake · 2026-10-21T15:49:01Z

@bayliffe - thank you so much for such a comprehensive review, I really appreciate the time you've put into this.

I've made a lot of changes off the back of this, which I think makes the script much better, but I realise also is quite a significant refactor... so it's probably not the easiest to re-review.

With that in mind, the things I would really appreciate your thoughts on:

I've reworked the implementation into a plugin following your earlier suggestion. Does this match what you had in mind?
Unit enforcement. I have my own _require_metre_units method, because I couldn't spot something that did what I wanted elsewhere in the repo. Wanted to check that this is a reasonable approach / if I'd missed something.
I've put a lot of effort into improving the readability of the code, docstrings, inline comments, variable names, etc. Also added extended documentation (this is a great option, thanks for pointing out). Is this easier to follow?

Thanks again 😊

gavinevans

Thanks @mo-AliceLake 👍

I'm just adding a brief comment about the plugin style, as requested. I'm not commenting on the science in any way. I'd say that the plugin mainly looks like I would expect, but I've noted a few things for consideration below.

gavinevans · 2026-10-23T16:48:15Z

+        - apply the correction
+    """
+
+    def __init__(


Just a brief comment on the general structure of this plugin, I'm not commenting on the science in any way. From a plugin layout perspective, I would expect:

The process method to be the last method in the class.

Cubes to be passed into process method and configurable variables to be passed into the __init__. In this plugin, it seems like only target_height_levels could fall into that category. It seems like there are potentially a lot of variables that could be exposed in the __init__ but these must currently rely on the default values specified in the various methods e.g. _fit_log_wind_profile.

I'd expect that when calling methods on the class from within the class, the self.method_name style would be used, rather than OrographicWindCorrection.method_name.

The plugin does seem complicated scientifically, so you could consider splitting the class into multiple classes. There seem to be 6 steps to the approach based on the documentation, so you could split it like that, or you could split it into differently. I definitely don't know enough about the approach to suggest a coherent way to split the plugin.

Thank you @gavinevans , this is incredibly helpful 😊

I have refactored (again!) following your suggestions: now has multiple classes, decided best split as _TerrainDescriptionMethods, _WindProfileMethods and _TerrainResponseMethods.

bayliffe

Co-pilot notes that:

Target heights are not explicitly checked for positivity, duplicates, or being within the source profile’s range. In particular, out-of-range targets can result in non-finite output rather than a targeted exception.

It looks as though out of range target height levels (500m turbine?!) could lead to nan wind speeds, so you may want to include a check against these.

Thanks for the refactor, I think this much better fits our typical structure. I've not revisited the science code as I believe that is essentially unchanged.

bayliffe · 2026-10-29T11:16:06Z

+                "uncorrected_wind_cube must include heights up to at least 300 m "
+                "above ground level, but max height provided is "
+                f"{np.max(self.original_height_levels)} m."
+            )


You've not checked that the lower limit is present and I'm pretty sure you need 0m for this method to work so you can figure out your roughness, is that right? Should a check therefore be included?

Actually I need at least two levels 0 <level height <= 300 (scientifically probably more than two, but I think that is up to a user to know that it will not interpolate well if they're passing in sparse data?!).
e.g. wind_speed_on_height_levels from StaGE starts at 5m (not 0m).

I've made the docstrings more explicit about this requirement and included a check for this.

bayliffe

The tiniest of questions, but I think this looks good. Thanks @mo-AliceLake.

bayliffe · 2026-11-02T16:12:09Z

    ),
    # Wind
    "wind_speed": Bounds((0, 50), "m s^-1"),
+    "wind_speed_at_hubheight": Bounds((0, 50), "m s^-1"),


I think this should be a suitable limit (~112mph), but I just want to check whether at ~300m we might need more headroom given that the wind is essentially unaffected by surface friction?

I did actually think about this (and convert it to mph to get a sense check) - the honest answer is that without obs I had no idea, and just thought "112mph feels quite big".

I've now (this week!) got some obs data for a wind farm in Scotland and for a wind farm in Northern Ireland for 2024, so had a look in there as a sense-check, and they max at 52m/s, so yes I definitely need to increase this. Have chosen 100 m/s. Thank you for flagging. 🙂

bayliffe

Thanks for all the work on this @mo-AliceLake, it's looking really good. Happy to approve.

mo-AliceLake and others added 16 commits April 30, 2026 10:33

Merging master

2857850

Making better match VMM

05b863c

Adding fallback

7f2335a

Adding positive only correctiosn

05d0093

Adding changes to use height profiles

8281d42

Updating CLI etc

5fb629e

Updating CLI orders to try and make workw ith script

bae2d5c

Slicing by realization intersection

5211c26

Cropping wind profile to reduce unecessary memory useage

1599dfd

Improving 'is close?' assertion

ed20812

Splitting to process by height level

b7994be

Fixing crop to allow for reference heights

dede866

Experiment 1

52a3dd0

Updating tests

f129cbb

mo-AliceLake marked this pull request as ready for review September 2, 2026 16:09

mo-AliceLake changed the title ~~Feature new wind orographic corrections~~ Implement new wind orographic correction method Sep 2, 2026

mo-AliceLake and others added 3 commits September 2, 2026 17:10

Merge branch 'master' into feature_new_wind_orographic_corrections

cf43db7

Updating checksum for smaller test files

4bfb268

Updating checksums

1ea8ff5

mo-AliceLake mentioned this pull request Sep 3, 2026

Wind orographic correction test data metoppv/improver_test_data#148

Merged

MoseleyS assigned mo-jbeaver Sep 29, 2026

mo-jbeaver requested changes Oct 5, 2026

View reviewed changes

mo-jbeaver assigned mo-AliceLake and unassigned mo-jbeaver Oct 5, 2026

Addressing PR review

73d4b62

Accounting for minimum height of 300

32c8894

Updating test values

26f500e

mo-AliceLake added 2 commits October 8, 2026 10:49

Updating acceptance tests

e747c73

Merging master

069d1d8

mo-jbeaver previously approved these changes Oct 9, 2026

View reviewed changes

bayliffe reviewed Oct 14, 2026

View reviewed changes

bayliffe requested changes Oct 15, 2026

View reviewed changes

Addressing review comments, refactoring as plugin.

4914c70

mo-AliceLake dismissed mo-jbeaver’s stale review via 4914c70 October 21, 2026 15:22

mo-AliceLake added 2 commits October 21, 2026 16:32

Tidying tests

45bea69

Adding extended documetnation

05892fc

gavinevans reviewed Oct 23, 2026

View reviewed changes

Splitting out classes as suggested

a15dd9e

bayliffe reviewed Oct 29, 2026

View reviewed changes

Addressing review comments

5a4a370

bayliffe reviewed Nov 2, 2026

View reviewed changes

Increasing bounds

f908f95

bayliffe approved these changes Nov 3, 2026

View reviewed changes

Merge branch 'master' into feature_new_wind_orographic_corrections

4420f05

bayliffe merged commit 5ba0ae8 into master Nov 3, 2026
7 checks passed

Conversation

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

Uh oh!

Description

Testing

Test coverage

Uh oh!

mo-jbeaver 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!

Uh oh!

Uh oh!

Uh oh!

mo-AliceLake commented Oct 5, 2026

Uh oh!

mo-AliceLake commented Oct 6, 2026

Uh oh!

mo-jbeaver commented Oct 7, 2026

Uh oh!

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

Uh oh!

Uh oh!

mo-jbeaver 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

gavinevans 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!

bayliffe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mo-AliceLake commented Sep 2, 2026 •

edited

Loading

mo-AliceLake commented Oct 8, 2026 •

edited

Loading

mo-AliceLake commented Oct 21, 2026 •

edited

Loading