Refactor code implementing initial chi transform with respect to North Pole and Schmit stretching

[Ricky Wong (mo-rickywong)]

PR Summary

Sci/Tech Reviewer: Thomas Bendall (@tommbendall)
Code Reviewer: Mike Hobson (@mike-hobson)

While generating the chi fields from meshes, the routine init_chi_transform is called from init_fem. The implementation of this is rather complicated and has in the past resulted in changes to test-suite KGOs if altered. This PR refactors/simplifies the implementation.

Issues with the existing implementation.

The initial chi transform as it stands:

• Passes in a mesh collection by argument (when it is available in module scope), only to extract the 1st mesh (which is only dependent on load order) to obtain the North Pole, Null Island, Equator latitude from the mesh. It doesn't allow for the case where there may be different meshes in the collection such as with the lfric2lfric application.
• The variables in the module init_chi_transform_mod are uninitialized and require that init_chi_transform is called in order to prevent random data. The transform is only for certain meshes (spherical geometry), however, omitting the call to init_chi_transform when not required still resulted in some KGO changes in the test-suite tasks.
• While the initialization is required only once, calls are made from init_fem for each mesh in the mesh_collection.

PR changes

• Simplify the API, only the key reference points are sent to the routine, north_pole, null_island, equator_latitiude. The determination of what those values are should be made by the application, though for the purposes of this PR, that determination is still applied in init_fem.
• Initialise (on declaration) and finalise the settings in init_chi_transform_mod so that they are for the default case, i.e. unrotated, unstretched mesh, to prevent random data.
• Only require the init_chi_transform to be called once for spherical geometry meshes. Even then its only necessary if the mesh has been rotated or stretched. This removes the bulk of required calls to init_chi_transform in the unit-tests routines.

Out-of-Scope issue

The current change refactors and tidies the existing code. Though there is still the issue of assuming that all spherical meshes in an application will use the pole/stretching criteria held in the module sci_chi_transforms_mod. Specifically in the general case with lfric2lfric where source/destination meshes could potentially have different pole location/stretching criteria.

Linked PRs

• linked Changes to Apps repository impacted by refactoring of init_chi_transforms lfric_apps#749

Code Quality Checklist

• I have performed a self-review of my own code
• My code follows the project's style guidelines
• Comments have been included that aid understanding and enhance the readability of the code
• My changes generate no new warnings
• All automated checks in the CI pipeline have completed successfully

Testing

• I have tested this change locally, using the LFRic Core rose-stem suite
• If required (e.g. API changes) I have also run the LFRic Apps test suite using this branch
• If any tests fail (rose-stem or CI) the reason is understood and acceptable (e.g. kgo changes)
• I have added tests to cover new functionality as appropriate (e.g. system tests, unit tests, etc.)
• Any new tests have been assigned an appropriate amount of compute resource and have been allocated to an appropriate testing group (i.e. the developer tests are for jobs which use a small amount of compute resource and complete in a matter of minutes)

trac.log

Test Suite Results - lfric_core - RefactorChiTrans/run1

Suite Information

Item	Value
Suite Name	RefactorChiTrans/run1
Suite User	ricky.wong
Workflow Start	2026-09-03T07:04:11
Groups Run	developer

Dependency	Reference	Main Like
lfric_core	mo-rickywong/lfric_core@RefactorChiTrans	False
SimSys_Scripts	MetOffice/SimSys_Scripts@cab3315	True

Task Information

✅ succeeded tasks - 433

Security Considerations

• I have reviewed my changes for potential security issues
• Sensitive data is properly handled (if applicable)
• Authentication and authorisation are properly implemented (if applicable)

Performance Impact

• Performance of the code has been considered and, if applicable, suitable performance measurements have been conducted

AI Assistance and Attribution

• Some of the content of this change has been produced with the assistance of Generative AI tool name (e.g., Met Office Github Copilot Enterprise, Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the Simulation Systems AI policy (including attribution labels)

Documentation

• Where appropriate I have updated documentation related to this change and confirmed that it builds correctly

PSyclone Approval

• If you have edited any PSyclone-related code (e.g. PSyKAl-lite, Kernel interface, optimisation scripts, LFRic data structure code) then please contact the HPC Optimisation Team

Sci/Tech Review

• I understand this area of code and the changes being added
• The proposed changes correspond to the pull request description
• Documentation is sufficient (do documentation papers need updating)
• Sufficient testing has been completed

(Please alert the code reviewer via a tag when you have approved the SR)

Code Review

• All dependencies have been resolved
• Related Issues have been properly linked and addressed
• CLA compliance has been confirmed
• Code quality standards have been met
• Tests are adequate and have passed
• Documentation is complete and accurate
• Security considerations have been addressed
• Performance impact is acceptable

[DanStoneMO]

I mentioned in the linked Apps PR that I was having some trouble with lfric-jedi for this change. Steven Sandbach (@ss421) was able to isolate it to this if statement.

The logic here requires that the mesh in prime_mesh_name is already set up. This is true most of the time, but lfric-jedi uses shared model configs. In tests using multiple resolutions, the non-prime mesh is set up first and fails when this tries to use the prime mesh here.

This can be fixed for lfric-jedi by dropping the if statement and always using all_mesh_names(1), though I have not tested what effect this would have on the Lfric side of things. Would it be possible to apply this change?

[Thomas Bendall (tommbendall)]

Oh and Ricky Wong (@mo-rickywong) it would be good if you could respond to this!

[Ricky Wong (mo-rickywong)]

#463 (comment)

As in the above response which references the need of base_mesh_config_mod to be application controlled. this is another example of where there is an assumption that base_mesh_config fits all. Jedi/Lfric2Lfric should be deciding which mesh that they are using to determine the chi_transform and that done before calling the init_fem routine.

Doing this in the application and being controlled by the application avoids the assumption that the 1st mesh in the collection is correct and allows apps which use more than one mesh to have some level of control.

Having spoken with Thomas Bendall (@tommbendall) there would be a long-term desire for the chi-transform matrices (or variables that determine it) to be passed from the mesh to the kernels. How that looks is not set yet. I'm not keen on using all_mesh_names(1) as it could be anything and might not be appropriate for jedi. While all_mesh_names(1) may work for jedi, it's not clear to me why, is it actually doing what jedi is expecting?

[Ricky Wong (mo-rickywong)]

This can be fixed for lfric-jedi by dropping the if statement and always using all_mesh_names(1), though I have not tested what effect this would have on the Lfric side of things. Would it be possible to apply this change?

The case for lfric2lfric/Jedi, at this point in the code is either they have not had an entry for &base_mesh or the prime_mesh has not been setup. Using all_mesh_names(1), sidesteps the issue but doesn't actually tell you whether the code is doing what you want. Even if it is (giving sensible results) it's not clear from all_mesh_names(1). all_mesh_names(1) is very fragile, a deviation in the order in which meshes are created could change/break your results. If init_chi_transform is used once and then assumed to be applied to all, it makes sense that an application specifies the mesh it is using and it knows why it's using it. Giving a specific mesh, removes the dependency on order of creation and ensures that the mesh was actually create i.e. you wouldn't have a mesh object otherwise.

The short answer is you could put back all_mesh_names(1), just to be clear, that would only maintain the status quo. The fragility of using all_mesh_names(1) still exists.

[DanStoneMO]

Steven Sandbach (@ss421) what are your thoughts on this? I am a bit out of my depth on this one

[Steven Sandbach (ss421)]

The existing change is not ideal, using all_mesh_names(1) is very fragile (especially in a routine that could be called by any applciation) and should instead to be a based off a specific mesh (chosen by the application). The use of prime_mesh_name is only there because that is what most apps currently use, I'm not sure which meshes lfric2lfric or jedi would be using.

Did you see my alternative suggestion? It would be possible to check it exists and if not fallback to: all_mesh_names(1) which it would do only for us (and maybe lfric2lfric...). It sounds like that should not be an issue for JEDI according to what you and Tom have said?

Another alternative would be to explicitly pass in the name which we can certainly do. That could be mandated or optional and then default back the config prime_mesh_name.

It's also unclear to me why prime_mesh_name is an issue for jedi. The mesh collection is global and all meshes could have easily been loaded before called init_fem. It would suggest something to do with the way JEDI is structured.

Its an issue because we use one config where we need to use two configs but are not at he point where we can do that. Yes its the way JEDI is setup. We are effectively running two models where we have:

1. The linear model configuration running at the lower resoluion
2. The Non linear model configuration running at the higher resolution. At the moment, the non-linear model requires only enough to setup IO as it is run by reading a previously run lfric-atm.

The configuration we are using is valid for the linear model and so has the lower resolution defined. The problem comes because we setup the NL model first.

[Ricky Wong (mo-rickywong)]

There is a requirement added as part of the LFRic-PAO projects to work with multiple configurations so when we get to that point then this issue will go away.

How does it go away, I would have though it might be potentially more risky if anything.

[Ricky Wong (mo-rickywong)]

In any case, I've reverted to using all_mesh_names(1). and testing these linked PRS which is all still green. However, leaving all_mesh_names(1) is not viable long term

[Steven Sandbach (ss421)]

How does it go away, I would have though it might be potentially more risky if anything.

Yes there is a risk that the two model geometry configurations are no longer compatible. This is something I have asked about before and has been discussed but there is no solution to the problem.

I have added it to the requirements document but don't have a good way to handle this case personally. So the risk moves from one where we have no control over the mesh that is selected here in some cases to a need to ensure the two geometry are compatible. When we run the full NL and TLAD together we will need two separate configurations.

[Steven Sandbach (ss421)]

In any case, I've reverted to using all_mesh_names(1). and testing these linked PRS which is all still green. However, leaving all_mesh_names(1) is not viable long term

OK thankyou.

[Thomas Bendall (tommbendall)]

There is a fundamental problem that the sci_chi_transform_mod was designed for a code base with a single geometry/topology/stretch factor/rotation, and can't handle apps or setups with multiple meshes of different type.

This PR takes a step towards resolving that by changing the API to init_chi_transforms so that the information required for coordinate transforms is not extracted from the mesh_collection, but is instead set by arguments that are directly passed in.

Although init_chi_transforms previously only did something for stretched/rotated meshes with a spherical geometry, it was previously compulsory (so that unset data values weren't used). My only concern is that by making the init_chi_transforms optional, a developer could accidentally break rotated/stretched tests.

The likely full solution to the overall problem would be to remove init_chi_transform entirely, and go through the pain of passing the north_pole, null_island and equatorial_latitude parameters to all kernels that involve a coordinate transform. As the developer says, this is left for future work which makes sense.

I'll be happy to approve this -- I only have specific suggestions around a spelling error of "Schmidt".

[Ricky Wong (mo-rickywong)]

Typos corrected.

[Thomas Bendall (tommbendall)]

Thanks for making these changes, and I'm glad we've reached a practical solution to avoid breaking JEDI code.

Happy for this to pass science review

[Thomas Bendall (tommbendall)]

Mike Hobson (@mike-hobson) this is ready for code review