Check horizontal grid before regridding

[BenMGeo]

Check if horizontal grid of cube is same as of target grid before regridding. Exits on first difference.
Note: No warning or feedback, if regridding is skipped as there is no logging functionality implemented in file yet.

Tasks

• Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
• This pull request has a descriptive title that can be used in a changelog
• Add unit tests
• Public functions should have a numpy-style docstring so they appear properly in the API documentation. For all other functions a one line docstring is sufficient.
• If writing a new/modified preprocessor function, please update the documentation
• Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
• Codacy code quality checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.

---

Closes #485

[BenMGeo]

I'm not sure why the tests fail. Is it, because the tests assume different coordinate names?

[valeriupredoi]

@BenMGeo the test is failing beause the first conditional in your function is failing ie cube.coord(coord).shape is a MagicMock object eg <Mock name='mock.coord().shape' id='140630128663888'> that differs from cube1 to cube2. In any case, please don't compare bounds at regrid level, especially horizontal regrid level, and here's why:

• cubes that get passed to the regrid module already have been checked on bounds existence by the CMOR checks and fixes module
• your conditional is way too strict ie

                if (c1bndval != c2bndval
                        and not np.allclose(c1bndval, c2bndval)):

will fail many times since bounds are not always equal and the differences can be of order 1 that will immediately fail allclose test since allclose is sensitive to <1e-5 see https://docs.scipy.org/doc/numpy/reference/generated/numpy.allclose.html

• regridding should happen even if latitude or longitude are not x-y coord names -> your first conditional will return coord not found error
• as a good practice, if we really want to allow for bounds checks we should introduce it as a separate regrid preprocessor option that can be turned on or off by the user, and executed part of the regrid wrapper in preprocessor/__init__.py

🍺

[valeriupredoi]

please see my comment 🍺

[valeriupredoi]

right - so I totally misinterpreted the purpose of this PR - just saw the issue related to it 😁 For that case there is no need to go this deep in checks (note that the comments above still stand but they are irrelevant wrt to the purpose of the PR) - you can just check if cube == target_grid provided an extra check that target_grid is a cube 🍺

[bouweandela]

It would be nice if you could also add a unit test for the new functionality

[bouweandela]

I'm not sure why the tests fail. Is it, because the tests assume different coordinate names?

The faling test is trying to check that the following code is executed and the coord method is called with the correct arguments:

ESMValCore/esmvalcore/preprocessor/_regrid.py

Lines 237 to 238 in 637bd47

	xcoord = target_grid.coord(axis='x', dim_coords=True)
	ycoord = target_grid.coord(axis='y', dim_coords=True)

with the correct arguments as on the line above. The reason that the test fails, is that you have added an extra call to the coord method with a different argument 'latitude' here:

ESMValCore/esmvalcore/preprocessor/_regrid.py

Lines 293 to 295 in 77a2556

	for coord in ['latitude', 'longitude']:
	# Compare shapes.
	if cube1.coord(coord).shape != cube2.coord(coord).shape:

[BenMGeo]

I'm not sure why the tests fail. Is it, because the tests assume different coordinate names?

The faling test is trying to check that the following code is executed and the coord method is called with the correct arguments:

ESMValCore/esmvalcore/preprocessor/_regrid.py

Lines 237 to 238 in 637bd47

	xcoord = target_grid.coord(axis='x', dim_coords=True)
	ycoord = target_grid.coord(axis='y', dim_coords=True)

with the correct arguments as on the line above. The reason that the test fails, is that you have added an extra call to the coord method with a different argument 'latitude' here:

ESMValCore/esmvalcore/preprocessor/_regrid.py

Lines 293 to 295 in 77a2556

	for coord in ['latitude', 'longitude']:
	# Compare shapes.
	if cube1.coord(coord).shape != cube2.coord(coord).shape:

So, I assumed that usually the preprocessor already checked for these specific coord names before. I could simply add x and y, as alternative. Reading the coordinates is an issue as we would also read time and z-coord, if available.

[BenMGeo]

right - so I totally misinterpreted the purpose of this PR - just saw the issue related to it grin For that case there is no need to go this deep in checks (note that the comments above still stand but they are irrelevant wrt to the purpose of the PR) - you can just check if cube == target_grid provided an extra check that target_grid is a cube beer

This does not catch the case that a self defined target_grid is fitting the cube in the function "accidentally". This is why I tried to include the checks on a different level.

[BenMGeo]

It would be nice if you could also add a unit test for the new functionality

Will do. But I wanted to understand the failing of the current tests before.

[bouweandela]

So, I assumed that usually the preprocessor already checked for these specific coord names before.

Yes, the preprocessor has checked that the coordinates have the correct names, so I think it's probably fine to use the names to find the horizontal coordinates.

I could simply add x and y, as alternative.

It would be good if you could have a careful look at the failing test. While simply using the same arguments in the second call to the coord method might make the test pass, I'm not sure it will still test what the author of the test intended, i.e. that this code is correctly executed:

ESMValCore/esmvalcore/preprocessor/_regrid.py

Lines 237 to 238 in 637bd47

	xcoord = target_grid.coord(axis='x', dim_coords=True)
	ycoord = target_grid.coord(axis='y', dim_coords=True)

[bouweandela]

Hi Ben,

Thanks for adding a first test. I changed it a bit, so the check if the grids are the same always returns False in the existing tests. If you would like more information on mocking, you can have a look at the unittest.mock documentation. It may seem daunting at first, but actually it's not that complicated, the basic idea is that you replace parts of your code with Mock objects, which do nothing at all (except maybe return a value, specified through the side_effect argument/attribute). Mocking can help to make tests more modular, that is why it is a popular technique.

It would be really great if you could also add a test that shows that cubes with a different grid actually get regridded to the target grid, while cubes with an identical grid are not regridded at all. There should be no need for mocking to do that ;-)

[BenMGeo]

It would be really great if you could also add a test that shows that cubes with a different grid actually get regridded to the target grid, while cubes with an identical grid are not regridded at all. There should be no need for mocking to do that ;-)

I thought that this is already included in the tests that are there at the moment (excluding shifted bounds).

[bouweandela]

I thought that this is already included in the tests that are there at the moment (excluding shifted bounds).

I checked and indeed there are tests that regridding works in tests/integration/preprocessor/_regrid/test_regrid.py. However, for the new functionality you added, i.e. regridding is skipped if the grids are the same, there is of course no test yet. Maybe you could add it there?

[bouweandela]

Friendly ping @BenMGeo Any plans to complete this pull request?

[BenMGeo]

Friendly ping @BenMGeo Any plans to complete this pull request?

Planned for the next 2 weeks while processing huge data. hopefully can implement reasonable tests in the end.

[stefsmeets]

Hi @BenMGeo , this PR has not seen much activity lately. No worries! Since it is so close to being finished, I hope you do not mind that I help out to get it over the finish line! 👍

[stefsmeets]

Hi @bouweandela and @valeriupredoi , from my understanding there are still two open issues with this PR:

It would be really great if you could also add a test that shows that cubes with a different grid actually get regridded to the target grid, while cubes with an identical grid are not regridded at all. There should be no need for mocking to do that ;-)

I have added a test for this case (test_regrid_is_skipped_if_grids_are_the_same).

Yes, that should be fine. Or you could just make one of the coordinates of equal shape but with different bounds, then you also have this case covered.

I added a test case with the same shape, but different bounds both lat and lon.

I also refactored the new tests a bit to use pytest instead of the old unittest library. Could you have a look whether your concerns have been addressed?

[bouweandela]

Thanks for the contribution @BenMGeo and thanks for wrapping this up @stefsmeets! 🥳

Outdated

[BenMGeo]

Thanks Peeps for finishing this.
I'm sorry that I answer so late. Hard time to contribute further, these days :(