160 rotating coords by alasdair-roy · Pull Request #163 · MetOffice/ANTS

alasdair-roy · 2026-07-23T13:08:16Z

Closes #160

To be completed prior to review request and updated as required during the review process.

If the answer to an item on the list is not applicable, feel free to replace the checkbox with 'N/A' to give extra clarity.

All developers are reminded to follow the ancil working practices

Branch

Related branches (e.g. ancillary-file-science):

[please link any related branches here]

ANTS rose stem logs

[https://cylchub/services/cylc-review/taskjobs/alasdair.roy/?suite=ANTS_160%2Frun2]

ancillary-file-science rose stem logs

[please enter the workflow name here as it has been run e.g. ancillary-file-science/run1]

Testing

For core ANTS only tests, the bare minimum that will be accepted is the group=unittests but many, if not most, changes will need to test other groups to ensure they meet reviewer expectations. In general, it should be possible and is advised to run the group=all group prior to review submission as this will catch any consequential issues. Additionally you must run the ancillary-file-science tests, pointing at your branch, with group=all to capture any behaviour changes affecting Science codes.

If your change will alter existing science results, you will need to seek appropriate Scientific validation and confirm that the model has been initialised with your new development. Inspecting a change in xconv/pyplot/visualiser of choice is not sufficient to demonstrate the model can be initialised from your file.

Impact of change

This will maintain results for ANTS cylc vip ./rose-stem -z group=all tests
This will this maintain results for ancillary-file-science cylc vip ./rose-stem -z group=all tests
If this change adds a new capability, evidence has been supplied to show testing of ancillary generation across different resolutions e.g. For global ancillary generation capabilities for use in NWP n1280e is expected to have been tested
This change has significantly impacted required resources (runtime and memory) in existing ancillary generation (if yes, give details)
This change alters existing ancils

Add further comments/details for your reviewers here on the impacts of the change......

Approvals for this change

I have approval from the ANTS core development team for these changes

New functionality further testing

If adding new functionality to existing codes, I confirm that the new code doesn't change results when it is switched off and ''works'' when switched on
Unittests have been added
Rose stem tests have been added for any new functionality
If adding new functionality please confirm that the new code compares across different standard decompositions.
I have not encountered any failures in my rose-stem output(s)
These tasks must succeed for your ticket to pass review.
I have remembered to run the code style check tasks/tools

Add details of any further testing here.

Other

I have read the Contributor Licence Agreement
I have added my name and affiliation to the Contributors list if I am not already on there in this PR.
The issue labels, milestones, etc. are correct
Links to all related issues have been provided in the pull request description
I have requested a code reviewer
Source data has been added or changed - please include a link to the license


I confirm that all code is my own and that my contributions are not subject to copyright or license restrictions (see Contributor Licence Agreement).	Alasdair Roy
I confirm I have not knowingly violated intellectual property rights (IPR) and have taken sensible measures to prevent doing so, including appropriate attribution for usage of Generative AI. I confirm that this work is my own, and I understand that it is my responsibility to ensure I am not violating others’ IPR. This includes taking reasonable steps to ensure that all tools used while creating this contribution did not infringe IPR.	Alasdair Roy

Please add any further notes here. If Generative AI tools have been used, a brief summary (e.g. "Github copilot used to add extra unittests") should be provided.

Rose stem logs

Please copy in the contents of your trac_status.log file(s) below (found in the cylc-run directory for your rose stem run) to your rose-stem testing here. Note: if your changes lead to a change in answers, you must run cylc vip ./rose-stem -z group=all to help ensure all affected configurations has been flagged up.

Tue 18 Aug 11:00:34 BST 2026 Git status: [ "" ] Commit: 368b878

Test Results - Summary

tasks	total
succeeded	65

Test Results - Detail

task	status
install_cold	succeeded
ancil_general_regrid_grid_to_n48_namelist_split0	succeeded
ancil_general_regrid_with_time_constraint_spiral_split2	succeeded
black	succeeded
ancil_general_regrid_grid_to_grid_kdtree_split1	succeeded
ancil_general_regrid_invert_mask_latitude_weighted_kdtree_split2	succeeded
ancil_general_regrid_3d_to_3d_with_extrapolation_split2	succeeded
ancil_2anc_split2	succeeded
ancil_general_regrid_grid_to_n48e_namelist_split1	succeeded
ancil_general_regrid_invert_mask_latitude_weighted_kdtree_split0	succeeded
ancil_2anc_split1	succeeded
ancil_fill_n_merge_invert_mask_kdtree	succeeded
ancil_general_regrid_3d_to_3d_with_extrapolation_split0	succeeded
ancil_general_regrid_grid_to_n48_namelist_split2	succeeded
ancil_general_regrid_grid_to_grid_kdtree_split2	succeeded
ancil_general_regrid_with_time_constraint_kdtree_split0	succeeded
ancil_create_ite_shapefile	succeeded
ancil_2anc_split0	succeeded
ancil_general_regrid_3d_to_3d_with_extrapolation_split1	succeeded
ancil_general_regrid_with_time_constraint_spiral_split0	succeeded
ancil_general_regrid_3d_to_3d_split1	succeeded
ancil_general_regrid_grid_to_variable_resolution_grid_split1	succeeded
unittests	succeeded
flake8	succeeded
ancil_general_regrid_grid_to_grid_latitude_weighted_kdtree_split0	succeeded
ancil_general_regrid_grid_to_n48e_namelist_split2	succeeded
build_docs	succeeded
isort	succeeded
ancil_general_regrid_grid_to_grid_spiral_split0	succeeded
ancil_general_regrid_invert_mask_spiral_split2	succeeded
ancil_general_regrid_invert_mask_kdtree_split2	succeeded
ancil_general_regrid_with_time_constraint_latitude_weighted_kdtree_split1	succeeded
ancil_general_regrid_grid_to_grid_latitude_weighted_kdtree_split1	succeeded
ancil_general_regrid_grid_to_variable_resolution_grid_split2	succeeded
ancil_general_regrid_with_time_constraint_spiral_split1	succeeded
ancil_general_regrid_grid_to_n48e_namelist_split0	succeeded
ancil_general_regrid_grid_to_n48_namelist_split1	succeeded
ancil_general_regrid_3d_to_3d_split0	succeeded
ancil_general_regrid_with_time_constraint_kdtree_split1	succeeded
ancil_general_regrid_3d_to_3d_split2	succeeded
ancil_general_regrid_with_time_constraint_kdtree_split2	succeeded
ancil_general_regrid_invert_mask_kdtree_split1	succeeded
ancil_general_regrid_grid_to_variable_resolution_grid_split0	succeeded
ancil_general_regrid_grid_to_grid_latitude_weighted_kdtree_split2	succeeded
ancil_fill_n_merge_invert_mask_spiral	succeeded
ancil_general_regrid_invert_mask_latitude_weighted_kdtree_split1	succeeded
ancil_fill_n_merge_invert_mask_latitude_weighted_kdtree	succeeded
ancil_general_regrid_grid_to_grid_kdtree_split0	succeeded
ancil_general_regrid_grid_to_grid_spiral_split2	succeeded
ancil_general_regrid_grid_to_grid_spiral_split1	succeeded
ancil_general_regrid_invert_mask_kdtree_split0	succeeded
ancil_general_regrid_invert_mask_spiral_split0	succeeded
ancil_general_regrid_with_time_constraint_latitude_weighted_kdtree_split0	succeeded
ancil_general_regrid_with_time_constraint_latitude_weighted_kdtree_split2	succeeded
ancil_general_regrid_invert_mask_spiral_split1	succeeded
ancil_fill_n_merge_land_cover_spiral	succeeded
ancil_fill_n_merge_land_cover_kdtree	succeeded
ancil_fill_n_merge_land_cover_latitude_weighted_kdtree	succeeded
rose_ana_2anc	succeeded
rose_ana_fill_n_merge	succeeded
rose_ana_general_regrid	succeeded
rose_ana_general_regrid_spiral	succeeded
rose_ana_general_regrid_kdtree	succeeded
rose_ana_general_regrid_latitude_weighted_kdtree	succeeded
linkcheck	succeeded

Add workflow_status.log contents for ancillary-file-science rose-stem here.

Josh Rackham (jrackham-mo)

The logic of this looks sensible to me. I've added some specific review comments inline. I also have a more general comment that we should probably save some metadata about the coordinate reference system alongside the shapefile. We should also check that this is picked up and respected on load by downstream applications (e.g. fill_n_merge).

In terms of how to implement this, it looks like the projection should be saved to a .prj file (wikipedia), and this is supported by GDAL.

Another general thing to think about is how/if we should handle polygons close to the boundary of the domain (i.e. the antimeridian). Looks like there is a library for this (antimeridian), but whether this is necessary for our purposes I don't know.

Josh Rackham (jrackham-mo) · 2026-07-27T08:22:36Z

+    valid_crs = not (
+        ants.utils.ndarray.allclose(
+            [grid_lon, grid_lat, pole_lon_rotation], [0.0, 90.0, 0.0]
+        )
+    )
+    if not valid_crs:


Just a minor style point here, there's a double negative here which makes it a little harder to follow. It might be better to have:

Suggested change

valid_crs = not (

ants.utils.ndarray.allclose(

[grid_lon, grid_lat, pole_lon_rotation], [0.0, 90.0, 0.0]

)

)

if not valid_crs:

invalid_crs = (

ants.utils.ndarray.allclose(

[grid_lon, grid_lat, pole_lon_rotation], [0.0, 90.0, 0.0]

)

)

if invalid_crs:

I suppose then you'd return not invalid_crs

I agree, I have changed this

Josh Rackham (jrackham-mo) · 2026-07-27T11:09:26Z

+    return rotated_points
+
+
+def _load_polygon_from_json(json_file, target_lsm_path, source_cube_path):


I think this function may have become complex enough to warrant splitting into two parts. The original load can probably stay as it was (perhaps returning a numpy array rather than the polygon object though, I think you said there was some difficulty with transforming the polygon object directly?), then have a separate standalone transform function that takes a loaded array, a source and a target cube, and does the transform.

I've changed the function to only load points from a json file

alasdair-roy · 2026-08-17T10:34:38Z

Here are the comments I've addressed:

For the antimeridian behaviour, if the distance between any two longitudes exceeds 180.0, then it's assumed that the polygon crosses the antimeridian. The approach I've used is not a rigorous as the antimeridian package but it seems to me that it creates multiple polygons which might not be desirable?
I've also added two extra checks. One checks the constructed polygon is valid (for example no intersecting boundaries) and the other checks the polygons have the same orientation before and after rotation.

The main comment I have not addressed is adding a .prj file, which I don't think is possible for rotated poles, see the issue
OSGeo/gdal#4726 .

Josh Rackham (jrackham-mo) · 2026-08-18T15:18:55Z

+    parser.add_argument(
+        "--source-cube",
+        type=ants.config.filepath_readable,
+        required=False,
+        help="Path to an iris cube which specifies the coordinate"
+        " system of the json file.",
+    )


I think it would be more appropriate to call this source-netcdf or just source. The iris cube only exists in memory, rather than being a file.

Josh Rackham (jrackham-mo) · 2026-08-18T15:21:08Z

+
+    if not polygon.is_valid:
+        raise ValueError(f"Polygon is invalid: {explain_validity(polygon)}")
+    if polygon.exterior.is_ccw != ccw_expected:


I'm unsure whether there are any cases where transforming the polygon would change the orientation? Is this something you've seen in testing?

Noting that we discussed adding a comment on why this check is included

Josh Rackham (jrackham-mo) · 2026-08-19T13:29:11Z

+    closed_lon = np.vstack([rotated_points, rotated_points[0, :]])
+    lon_diff = np.abs(closed_lon[:-1, 0] - closed_lon[1:, 0])
+
+    if np.any(lon_diff > 180.0):
+        neg_indices = np.where(rotated_points[:, 0] < 0)
+        rotated_points[neg_indices, 0] += 360.0


I think a comment would be good here to explain the logic, and mention it in the docstring (i.e. negative longitudes will be shifted by +360 degrees if a jump of 180 degrees is detected)

Could we also throw a warning here, or logger.info?

An additional check I'd like to see is if any of the transformed points fall outside of the target domain, and throw an error if they do.

I've included some comments to explain what is going on and also added a warning.

I also added a check for the transformed points landing in the target domain. I haven't included a check on if the polygon is defined on an equivalent domain (for example rotated 360.0) because it seems that fill_n_merge doesn't check for this.

Josh Rackham (jrackham-mo) · 2026-08-19T14:40:55Z

+        self.assertTrue(_validate_orientation(self.sphere_rotated_target_lsm))
+
+
+class Test__transform_coordinates(ants.tests.TestCase):


Could we add a test case using the ite polygon, transformed to a standard UK domain (pole_lat=37.5, pole_lon=177.5)?

It would also be good to have a test case with the target cube having a regional domain (i.e. non-global) to test that an error is raised if any of the transformed points lie outside the target domain.

Yes that's a good idea, I've added a test case for both

Josh Rackham (jrackham-mo) · 2026-08-19T14:42:15Z

+        longitude."""
+
+        true_lats = np.array([90, 45, 0, -45, -90])
+        true_lons = np.array([np.nan, -180.0, -180.0, -180.0, -180.0])


Can we double check if the nans are needed? If a point lies on the rotated pole, I think we should probably throw an error. Maybe we should check if nans appear in the output?

I've changed these tests to be more representative of what will actually be passed into the function. Rather than just a handful of points the passed points are in a square and the poles are no longer near the points.

Josh Rackham (jrackham-mo) · 2026-08-19T14:46:42Z

+@mock.patch("builtins.open", new_callable=mock.mock_open)
+class Test__load_points_from_json(ants.tests.TestCase):


I think we can remove this test

Josh Rackham (jrackham-mo) · 2026-08-19T14:49:03Z

General comment on the unit tests is that there's a lot of things created in the setup methods that are only used by one test. It might make it more readable to only create the cubes in the test you need them, unless the cubes are used by multiple tests.

I agree, I have tried to make the tests more readable

alasdair-roy · 2026-08-19T15:28:42Z

Also need to add a .txt file saving information on the crs (source and target)

alasdair-roy added 15 commits July 9, 2026 10:38

Placed sketch solution for rotating co-ordinate systems

e47273e

Allow points to be transformed to a rotated pole.

ca534fb

Testing with optional argument in rose-stem.

2d8879e

Added source cube as argument, added some tests for shapefile creation.

36a3819

Added more tests

a6a60eb

Changed source cube to optional and cleaned tests

64af015

Remove some comments that are not needed

5eed8dd

Improved comments in tests.

4448d86

Fixed issue causing polygon to invert

5d1a68a

Moved functionality out of load polygon to ease testing.

af32edf

Minor reformatting of docstrings.

fa4bbeb

Added docstrings to tests.

791118e

Changed some mocking in tests to improve readability.

4ad7412

Corrected spelling mistakes.

5ea3e01

Moved loading out of function to make testing easier.

c848fb5

alasdair-roy added the ✨ enhancement Feature request for new capability label Jul 23, 2026

alasdair-roy added this to the 4.0 milestone Jul 23, 2026

alasdair-roy marked this pull request as ready for review July 23, 2026 13:18

Josh Rackham (jrackham-mo) self-requested a review July 23, 2026 13:55

Josh Rackham (jrackham-mo) assigned alasdair-roy Jul 23, 2026

Josh Rackham (jrackham-mo) requested changes Aug 7, 2026

View reviewed changes

alasdair-roy added 6 commits August 10, 2026 10:26

Moved transformation out of polygon loading.

5f07ac1

Add code to address antimeridian crossing.

52dd428

Fixed using wrong points for antimeridian crossing.

8f9799c

Changed variable names to correct long misname.

2cc209b

Removed double negative.

2534d8b

Added extra checks and tests for polygon validity and orientation.

362d653

alasdair-roy requested a review from Josh Rackham (jrackham-mo) August 17, 2026 10:34

Renamed polygon variable name in test to match orientation.

368b878

Josh Rackham (jrackham-mo) requested changes Aug 19, 2026

View reviewed changes

alasdair-roy added 5 commits August 20, 2026 13:38

Addressed review on readability and docstrings.

e1d61ea

Added integration test for unrotated ITE.

8e9ae5b

Added new ite test and made other tests more consistent.

b76caf7

Added warning if the polygon lands outside of the local domain.

12f4707

Added polygon domain warning and added json metadata file.

613372e

		return rotated_points


		def _load_polygon_from_json(json_file, target_lsm_path, source_cube_path):

		self.assertTrue(_validate_orientation(self.sphere_rotated_target_lsm))


		class Test__transform_coordinates(ants.tests.TestCase):

		@mock.patch("builtins.open", new_callable=mock.mock_open)
		class Test__load_points_from_json(ants.tests.TestCase):

Uh oh!

Conversation

alasdair-roy commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Branch

Testing

Rose stem logs

Test Results - Summary

Test Results - Detail

Uh oh!

Josh Rackham (jrackham-mo) 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alasdair-roy commented Aug 17, 2026

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!

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!

alasdair-roy commented Aug 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alasdair-roy commented Jul 23, 2026 •

edited

Loading