Add configurable grid spacing relative tolerance by rachaelesler · Pull Request #2308 · metoppv/improver

rachaelesler · 2026-02-17T05:38:02Z

Addresses #2307

This feature allows users of the regrid CLI to supply a relative
tolerance for grid spacing checks, e.g.,

improver regrid <cubes> --rtol-grid-spacing=4.0e-3

Previously, this relative tolerance was hard-coded into a function.
The change in this commit enables users to use the regrid CLI with
grids that have minor discrepancies in latitude/longitude spacing.

The new --rtol-grid-spacing argument is only used for certain regrid
modes ("nearest-2", "nearest-with-mask-2","bilinear-2", and
"bilinear-with-mask-2").

The new --rtol-grid-spacing argument is optional and it takes a
default value. The default value is the hard-coded value that existed
previously. This means that existing systems that uses the regrid CLI
will not be affected by this change.

This changeset includes:

Add --rtol-grid-spacing argument to regrid CLI
Update the classes RegridLandSea and RegridWithLandSeaMask to
have a new attribute, rtol_grid_spacing
Update the calculate_input_grid_spacing() function in
improver/regrid/grid.py with a new argument, rtol, which is the
relative tolerance that will be used when calculating grid spacing.
Use constants for default grid spacing rtols (rather than magic
numbers)
Update calculate_grid_spacing() function in
improver/utilities/spatial.py with more input validation
Unit and acceptance tests for all of the above

Testing:

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

CLA

If a new developer, signed up to CLA

improver/constants.py

benowen-bom

Partial review: I'll check the remaining tests tomorrow. And sorry for the delay, I've been trying to test the best location for the constants (between other tasks).

benowen-bom · 2026-02-24T05:33:12Z

improver/utilities/spatial.py

+# Grid spacing relative tolerances
+RTOL_GRID_SPACING_TIGHT = 1.0e-5
+RTOL_GRID_SPACING_DEFAULT = 4.0e-5
+RTOL_GRID_SPACING_WARNING_THRESHOLD = 0.01
+


Thanks for moving this out of constants.py. I notice that including these here means that the regrid.py CLI can not import RTOL_GRID_SPACING_DEFAULT directly due to the issue with the numpy import.

A possible solution to this is to move these into regrid/_init_.py as this will enable them to be imported into the CLI without issue.

It also moves these grid-specific constants into the regrid directory, which seems like a reasonable place for them (given that is where they are primarily used).

I don't think an __init__.py file is the right place for these constants for a few reasons. The main one is that regrid/__init__.py is currently empty and I am guessing this is by design from the MO IMPROVER team. The official Python docs indicate that __init__.py should contain package initialisation code but constants don't really fit under the umbrella of package initialisation.

I don't think it's too much of an issue that the constants aren't available in the CLI code, because this is the same pattern used in many other IMPROVER CLIs.

I'm tempted to leave these constants as is now, and get feedback from the Met Office team if they have a better suggestion on where to put them.

No worries. I'll resolve the other issues raised under this item.

improver/cli/regrid.py

improver/regrid/grid.py

improver/regrid/landsea.py

improver/regrid/landsea2.py

benowen-bom · 2026-02-24T05:48:47Z

improver_tests/regrid/test_RegridLandSea.py

+    def test_run_regrid_with_tolerance_specified(self):
+        """Test masked regridding with relative grid tolerance specified."""
+        expected_data = 282 * np.ones((12, 12), dtype=np.float32)
+        test_cases = [1.0e-10, 1.0e-5, 4.0e-3, 7.0e-1]
+        for rtol in test_cases:
+            with self.subTest(rtol=rtol):
+                result = RegridLandSea(
+                    regrid_mode="nearest-with-mask-2",
+                    landmask=self.landmask,
+                    landmask_vicinity=90000,
+                    rtol_grid_spacing=rtol,
+                )(self.cube, self.target_grid.copy())
+                np.testing.assert_array_almost_equal(result.data, expected_data)
+


Given rtol_grid_spacing is only used to run a checking function, I'm not sure there is much value in the tests run above as they don't really don't do more than check that the class runs without failing.

Maybe this test could be broken down into three cases:

Test runs as expected (as above)

Test runs in case the warning is raised; it should warn, but still work

Test case where the tolerance is something bonkers which fails

The 1.0e-10 tolerance looks like a good candidate for the last one, but may require a test grid with a more exotic grid-spacing (one likely to suffer from precision issues).

I hear that, I could probably cut this down to a single test here (testing with just one custom rtol) as I've tested the three test cases you mentioned at the function level in improver_tests/regrid/test_grid.py. What do you think?

rachaelesler · 2026-02-25T05:25:26Z

@benowen-bom As discussed I will scale back the testing and add tests for the following:

Test that rtol is passed to dependent functions (when passed at the higher levels)
Test a case with a grid where the default rtol fails, but a larger rtol causes the grid to pass

benowen-bom · 2026-02-25T05:42:22Z

@benowen-bom As discussed I will scale back the testing and add tests for the following:

Test that rtol is passed to dependent functions (when passed at the higher levels)

Test a case with a grid where the default rtol fails, but a larger rtol causes the grid to pass

Thanks @rachaelesler. In looking over the rest of the code changes, I think scaling back the tests is worthwhile. Given we have not changed the functionality, rather provided a means to change a value utilised in the existing functionality, I think what should be tested is:

function still works (with default and with a non-default value set)
the function warns when the value passed is too large
the function raises an error for non-sensible values (negative spacing)
we can replicate the situation we are trying to address, ie. grid where default tolerance fails, but a larger (sensible) rtol allows this to pass.

In terms of checking the rtol is passed through for dependent functions, you could add a single test to each class that has been affected where 2values are passed in: sensible value and an excessively large one which should warn: the first case should pass without error and the second case should raise a warning.

With these tests, I think we have done the due-diligence without adding more coverage than is necessary (especially given the arg is used in a check function which has no material impact on the data).

rachaelesler · 2026-02-27T05:24:57Z

@benowen-bom As discussed I will scale back the testing and add tests for the following:

Test that rtol is passed to dependent functions (when passed at the higher levels)

Test a case with a grid where the default rtol fails, but a larger rtol causes the grid to pass

Thanks @rachaelesler. In looking over the rest of the code changes, I think scaling back the tests is worthwhile. Given we have not changed the functionality, rather provided a means to change a value utilised in the existing functionality, I think what should be tested is:
* function still works (with default and with a non-default value set)

* the function warns when the value passed is too large

* the function raises an error for non-sensible values (negative spacing)

* we can replicate the situation we are trying to address, ie. grid where default tolerance fails, but a larger (sensible) rtol allows this to pass.
In terms of checking the rtol is passed through for dependent functions, you could add a single test to each class that has been affected where 2values are passed in: sensible value and an excessively large one which should warn: the first case should pass without error and the second case should raise a warning.

With these tests, I think we have done the due-diligence without adding more coverage than is necessary (especially given the arg is used in a check function which has no material impact on the data).

@benowen-bom I think I have made most of the changes suggested above and this is ready for another review. Please let me know if there is additional coverage you think I should add.

rachaelesler · 2026-03-04T22:56:14Z

Hi @bayliffe, I believe this is ready for a review from the Met Office. I'm still new to IMPROVER development, so any feedback is welcomed. In particular, I am interested in your thoughts on whether the test coverage is sufficient, and where to put the constants used in regridding. Thanks!

rachaelesler · 2026-03-09T23:39:46Z

I've rebased on master

bayliffe · 2026-03-10T13:48:25Z

@rachaelesler apologies for not getting to this yet. I'm not well today, so I will try to get to it tomorrow.

rachaelesler · 2026-03-10T21:41:56Z

@rachaelesler apologies for not getting to this yet. I'm not well today, so I will try to get to it tomorrow.

No worries @bayliffe

Some of the tests are failing after a recent rebase on master, but this looks to be the case on other PR branches too. I could be wrong but I suspect the culprit for the failing tests is not this code, so it is good to review.

This feature allows users of the `regrid` CLI to supply a relative tolerance for grid spacing checks, e.g., ``` improver regrid <cubes> --rtol-grid-spacing=4.0e-3 ``` Previously, this relative tolerance was hard-coded into a function. The change in this commit enables users to use the `regrid` CLI with grids that have minor discrepancies in latitude/longitude spacing. The new `--rtol-grid-spacing` argument is only used for certain regrid modes ("nearest-2", "nearest-with-mask-2","bilinear-2", and "bilinear-with-mask-2"). The new `--rtol-grid-spacing` argument is optional and it takes a default value. The default value is the hard-coded value that existed previously. This means that existing systems that uses the `regrid` CLI will not be affected by this change. This changeset includes: * Add `--rtol-grid-spacing` argument to `regrid` CLI * Update the classes `RegridLandSea` and `RegridWithLandSeaMask` to have a new attribute, `rtol_grid_spacing` * Update the `calculate_input_grid_spacing()` function in `improver/regrid/grid.py` with a new argument, `rtol`, which is the relative tolerance that will be used when calculating grid spacing. * Use constants for default grid spacing rtols (rather than magic numbers) * Update `calculate_grid_spacing()` function in `improver/utilities/spatial.py` with more input validation * Unit and acceptance tests for all of the above Closes metoppv#2307

benowen-bom

Hi Rachael,

I've had another look over this PR. I think the testing is much more appropriate given the changes. I have one suggestion below, otherwise I'm happy with the changes.

benowen-bom · 2026-03-09T21:49:27Z

improver_tests/regrid/test_RegridLandSea.py

+        y_array = np.array(
+            [
+                -46.8,
+                -46.813496399,
+                -46.826994705,
+                -46.840493965,
+                -46.85399418,
+                -46.86749058,
+                -46.880988884,
+                -46.894488144,
+                -46.90798836,
+                -46.92148476,
+                -46.93498306,
+                -46.94848232,
+                -46.96198254,
+                -46.97547894,
+                -46.98897724,
+            ],
+            dtype=np.float32,
+        )
+        # Regularly spaced x points
+        x_array = np.linspace(100.0, 115.0, n_points, dtype=np.float32)
+        data = 282 * np.ones((15, 15), dtype=np.float32)
+        y_coord, x_coord = _construct_yx_coords_from_arrays(
+            y_array=y_array, x_array=x_array, spatial_grid="latlon"
+        )
+        dim_coords = _construct_dimension_coords(
+            data,
+            y_coord=y_coord,
+            x_coord=x_coord,
+        )
+        src_cube = Cube(
+            data.astype(np.float32),
+            standard_name="air_temperature",
+            units="K",
+            dim_coords_and_dims=dim_coords,
+        )


This is fine, but I'm wondering whether you could do something similar here to what was done for test_lat_lon_with_irregular_grid in test_regrid.py? That is, set up the cube and then alter the y_coord directly?

I appreciate the values above have been chosen to give the desired behaviour, but an alternate means could be to add and subtract delta to two of the coordinate values, where delta > rtol * mean(grid_spacing). That is:

y_coord = [y0, y1, y2 + delta, y3- delta, y4, ..., yn] ```. This would ensure `mean(grid_spacing)` is the same, but that one or more grid_spacing values exceed the rtol; additionally, the rtol used to set the delta value determines whether the function will pass or fail.

Great idea, I've applied your suggestion now and it has made the test much more concise.

rachaelesler marked this pull request as draft February 17, 2026 05:38

rachaelesler force-pushed the grid-spacing-rtol branch 3 times, most recently from ef01dc0 to d1cc63c Compare February 17, 2026 06:04

rachaelesler changed the title ~~Allow configurable relative tolerance for regrid~~ Add configurable grid spacing relative tolerance Feb 17, 2026

rachaelesler force-pushed the grid-spacing-rtol branch from d1cc63c to 7ad9c3e Compare February 17, 2026 06:12

rachaelesler commented Feb 17, 2026

View reviewed changes

improver/constants.py Outdated Show resolved Hide resolved

rachaelesler marked this pull request as ready for review February 17, 2026 06:21

rachaelesler requested a review from benowen-bom February 17, 2026 06:21

rachaelesler commented Feb 18, 2026

View reviewed changes

improver/constants.py Outdated Show resolved Hide resolved

rachaelesler force-pushed the grid-spacing-rtol branch from 8592766 to e99fda8 Compare February 19, 2026 00:07

benowen-bom reviewed Feb 24, 2026

View reviewed changes

rachaelesler force-pushed the grid-spacing-rtol branch from 3c9b6e6 to 9fc0fec Compare March 2, 2026 02:37

rachaelesler requested a review from bayliffe March 4, 2026 22:53

gavinevans added the MO review required PRs opened by non-Met Office developers that require a Met Office review label Mar 5, 2026

bayliffe self-assigned this Mar 9, 2026

rachaelesler force-pushed the grid-spacing-rtol branch from 9fc0fec to 0a9a45f Compare March 9, 2026 23:39

rachaelesler added 6 commits March 12, 2026 11:42

Move constants to improver.utilities.spatial

31e4544

Remove import from regrid.py

b33518a

Update warning threshold to 1%

1ba1f06

Update tests

256b1ca

Fix tests?

7f9163e

rachaelesler force-pushed the grid-spacing-rtol branch from ea49860 to 7f9163e Compare March 12, 2026 00:42

benowen-bom reviewed Mar 12, 2026

View reviewed changes

rachaelesler added 2 commits March 12, 2026 16:47

Simplify regrid tests per PR comments

43e40a5

Revert some changes

d533245

Conversation

rachaelesler commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benowen-bom left a comment

Choose a reason for hiding this comment

Uh oh!

benowen-bom Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

rachaelesler Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

benowen-bom Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benowen-bom Feb 24, 2026

Choose a reason for hiding this comment

Uh oh!

rachaelesler Feb 25, 2026

Choose a reason for hiding this comment

Uh oh!

rachaelesler commented Feb 25, 2026

Uh oh!

benowen-bom commented Feb 25, 2026

Uh oh!

rachaelesler commented Feb 27, 2026

Uh oh!

rachaelesler commented Mar 4, 2026

Uh oh!

rachaelesler commented Mar 9, 2026

Uh oh!

bayliffe commented Mar 10, 2026

Uh oh!

rachaelesler commented Mar 10, 2026

Uh oh!

benowen-bom left a comment

Choose a reason for hiding this comment

Uh oh!

benowen-bom Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

rachaelesler Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rachaelesler commented Feb 17, 2026 •

edited

Loading