Skip to content

Decouple base mesh geometry / topology enumerations from low level lfric code - #463

Open
Ricky Wong (mo-rickywong) wants to merge 8 commits into
MetOffice:mainfrom
mo-rickywong:DecoupleGeoTop
Open

Ricky Wong (mo-rickywong) wants to merge 8 commits into
MetOffice:mainfrom
mo-rickywong:DecoupleGeoTop

Conversation

@mo-rickywong

@mo-rickywong Ricky Wong (mo-rickywong) commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

PR Summary

Sci/Tech Reviewer: Thomas Bendall (@tommbendall)
Code Reviewer: mo-lucy-gordon

There are many places in the lfric core/apps core which use the mesh geometry / topology namelist settings to determine behaviour. These are taken from base_config_mesh_mod, however, this namelist was never intended to be used as a proxy test for what mesh a piece of code operates on. The code should be querying the mesh object itself, base_mesh_config_mod was only to test if the mesh in the file matched the expected configuration.

Existing code stills uses geometry/topopogy enumerations and these are tied to the base_mesh_config_mod. To decouple base_mesh_config enumerations, the enumeration access is moved to the mesh_type object. Some parts of the code may still be accessing geometry/topology from module scope without a clear access to the correct mesh_object . Until all parts of the code access geometry/topologyfrom the mesh object, local_mesh_mod will use base_mesh_config_mod and later on use fixed hardcoded enumeration values.

Eventually, base_mesh_config should be moved out of components and be under the control of the application.

Linked PRs

This PR is blocking

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 - DecoupleGeoTop/run1

Suite Information

Item Value
Suite Name DecoupleGeoTop/run1
Suite User ricky.wong
Workflow Start 2026-09-03T07:23:15
Groups Run developer
Dependency Reference Main Like
lfric_core mo-rickywong/lfric_core@DecoupleGeoTop 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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have any issues with the code itself, but I don't fully understand the motivation from the PR description. Is this to support apps like lfric2lfric which may have a single namelist but multiple meshes (with different geometry/topology)?

I think I understand that the reason that base_mesh_config_mod is now used in local_mesh_mod because it's too much work for a single PR to remove it? Removing geometry/topology from base_mesh_config would require all kernels still getting geometry/topology from base_mesh_config to be updated first -- but I do wonder if it would've been better to do that first? That would have avoided the confusing situation of infrastructure code now depending on namelists from components/driver, which is the consequence of this PR.

I also don't understand the suggestion in the description that the base_mesh_config should be moved out of LFRic-Core: it seems about the most core-y namelist there is!

Finally, please could you revert the changes to the log level in panel_decomposition_mod which are not related to this PR.

PS there is a typo in the title: geology -> geometry

Comment thread components/driver/source/driver_mesh_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/partition_mod.F90 Outdated
@mo-rickywong Ricky Wong (mo-rickywong) changed the title Decouple base mesh geology / topology enumerations from low level lfric code Decouple base mesh geometry / topology enumerations from low level lfric code Sep 14, 2026
@mo-rickywong

Copy link
Copy Markdown
Contributor Author

I don't really have any issues with the code itself, but I don't fully understand the motivation from the PR description. Is this to support apps like lfric2lfric which may have a single namelist but multiple meshes (with different geometry/topology)?

Yes, it's to support apps such as Lfric2Lfric, however, Lfric2lfric's issues only highlighted the problem. base_mesh was originally in driver with the assumption that applications would be using 1 mesh to run on (which falls over with apps like lfric2lfric). However, it was only to test that the mesh an application reads in from a file is valid for that applications intended configuration. After that, all the infrastructure is concerned about is the mesh that is passed to it. It is not aware of the application (or the applications configuration) that called it. It should only be concerned about the the mesh object that is passed to it.

In most cases, it works out to be the same (1 mesh, checked to match the application etc), though it's not appropriate. This practice of using the base_mesh_config_mod has been copied throughout which makes the issue a substantial amount of work to resolve.

I think I understand that the reason that base_mesh_config_mod is now used in local_mesh_mod because it's too much work for a single PR to remove it? Removing geometry/topology from base_mesh_config would require all kernels still getting geometry/topology from base_mesh_config to be updated first -- but I do wonder if it would've been better to do that first?

Doing that first would require all kernels to have the config_type plumbed to them and then geometry/topology accessed by the =config%base_mesh%...() method, only to have all that plumbing removed in a subsequent PR when accessing from the mesh that is already in the routine. Even with lfric_core completely changed over not using module scope, there is a large amount to files in apps that would still access geometry/topology from module scope.

That would have avoided the confusing situation of infrastructure code now depending on namelists from components/driver, which is the consequence of this PR.

That is temporary, it is taken care of in the preprocessing and uses the same enumerations as in base_mesh_config (until all code is ported across), so it is transparent to users. They can access the enumeration values from local_mesh_mod, mesh_mod or base_mesh_mod. Eventually it should be just mesh mod, but having them available means all the files don't have to be done in one PR.

I also don't understand the suggestion in the description that the base_mesh_config should be moved out of LFRic-Core: it seems about the most core-y namelist there is!

Base mesh is one of those, "sounded good at the time" type of namelists. It was the assumption that applications would only have one principle mesh. That's not really the case now. The driver layer should work on the mesh(es) that's its presented with, though it shouldn't really be deciding how an app configures the meshes for the app. The app should determine these then pass the meshes to infrastructure or components to work with. So the functionality of base mesh should be with the individual apps. They don't have to call it base mesh or could have multiple namelists to determine their needs. The functionality may be common among most apps, but that is a different issue and doesn't imply it is for infrastructure to manage.

Finally, please could you revert the changes to the log level in panel_decomposition_mod which are not related to this PR.

PS there is a typo in the title: geology -> geometry

@mo-rickywong

Ricky Wong (mo-rickywong) commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Log levels reverted on this PR

@mo-rickywong Ricky Wong (mo-rickywong) left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes made, back to scitech

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making those changes. mo-lucy-gordon this is ready for code review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants