Skip to content

Remove module scope access from LFRic Core driver/applications - #324

Merged
James Bruten (james-bruten-mo) merged 79 commits into
MetOffice:mainfrom
mo-rickywong:RmGlobalCfg_CoreDriver
Jul 1, 2026
Merged

Remove module scope access from LFRic Core driver/applications#324
James Bruten (james-bruten-mo) merged 79 commits into
MetOffice:mainfrom
mo-rickywong:RmGlobalCfg_CoreDriver

Conversation

@mo-rickywong

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

Copy link
Copy Markdown
Contributor

PR Summary

Sci/Tech Reviewer: Various
Code Reviewer: Andrew Coughtrie (@andrewcoughtrie)

  • Removes access to configurations variables from modules scope in components/driver and applications directories.
  • This had a knock on effect and required a range of namelist vafriables to be passed by argument down to components/science and components/lfric-xios directories. This in turn required numerous changes in the LFRic-apps repository. Resulting in a linked ticket to LFRic_apps.
  • Multigrid and tiling caused complications in the removal as related code and namelist variables were placed in inappropriate locations. Multigrid related code has been moved to multigrid_mod so it can easily be relocated to Gungho science at a later date and tiling properties have been moved above the driver code so it can be modified by individual applications if needed, this results in the init_mesh routine being only passed the inner halo tiling that is required of it. This required and upgrade macro. The effect of the macro applied to lfric_core is shown at: mo-rickywong@3d0cbe1
    Note: : Tiling is on main as being instantiated when constructing a mesh_type, instead it should be attached to a local_mesh_type, this is beyond the scope of this PR

Linked PRs

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)

The test-suite was run on a local branch after the update macro was applied. Test suite appears green

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 TCD 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

@mo-lucy-gordon mo-lucy-gordon left a comment

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.

The files from the list I have reviewed all look good.

Co-authored-by: Thomas Bendall <14180399+tommbendall@users.noreply.github.com>
Co-authored-by: Thomas Bendall <14180399+tommbendall@users.noreply.github.com>
Co-authored-by: Steve Mullerworth <26165365+stevemullerworth@users.noreply.github.com>
Co-authored-by: Thomas Bendall <14180399+tommbendall@users.noreply.github.com>
Co-authored-by: Andrew Coughtrie <24609575+andrewcoughtrie@users.noreply.github.com>
Co-authored-by: Matthew Hambley <MatthewHambley@users.noreply.github.com>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This all looks good to me now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks fine to me.

@mo-rickywong

Copy link
Copy Markdown
Contributor Author

Thomas Bendall (@tommbendall) Steve Mullerworth (@stevemullerworth)
Can't remember if all issues have been addressed, please can you check your requested re-reviews?

@mo-rickywong

Copy link
Copy Markdown
Contributor Author

I'm supportive of the changes in this PR and have discussed them with the developer during the development of this ticket.

The developer has changed the mesh_id argument to be the mesh for many runtime constants. I have no objections to this but do have concerns that since many runtime constants are left untouched, there is an inconsistency that could confuse developers (we now have to remember which runtime constants take mesh and which take mesh_id as an argument). The developer reasonably is limiting the scope of this PR and not touching the other runtime constants, but I would like to see a plan to address this -- ideally with an issue opened and someone assigned to it.

Also, I'm sorry to be fussy, but the developer has also applied his own style across various algorithm files, which means that some of these files now have inconsistent style. Although I do like some of the style changes, I am requesting that the developer reverts many of these changes for my code area. I would also prefer to have the line continuation & symbols set to be the 80th character of the line in sci_mapping_constants_mod.x90 but it's very difficult to do that through a suggestion.

I do appreciate the addition of variable names to the metadata for some kernels. I think it is more consistent with other kernels to include a space between the ampersand and exclamation mark so am also suggesting this.

#415 has been created to remove mesh_id usage from science components

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks fine now.

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 the style reversions. I'm happy to approve.

@mo-rickywong Ricky Wong (mo-rickywong) added this to the Summer 2026 milestone Jun 30, 2026
@james-bruten-mo
James Bruten (james-bruten-mo) merged commit 5d6b2db into MetOffice:main Jul 1, 2026
4 checks passed
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.

10 participants