Skip to content

Allow meshes to be renamed on load - #452

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

Ricky Wong (mo-rickywong) wants to merge 15 commits into
MetOffice:mainfrom
mo-rickywong:lfric2lfric_in_and_out

Conversation

@mo-rickywong

@mo-rickywong Ricky Wong (mo-rickywong) commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

PR Summary

Sci/Tech Reviewer: cjohnson-pi
Code Reviewer: Matthew Hambley (@MatthewHambley)

Access to meshes in input files produced from the mesh generators should contain unique mesh names by design. The mesh names are used as a more user-friendly means to reference meshes in downstream applications. This however becomes an issue when applications such as lfric2lfric are required to read in multiple files produced for other applications. Applications which generate their input files may have used mesh(es) with identical names (e.g. dynamics, multigrid_l1, etc) in their individual tasks which produce output.

Adopting a practice of naming a mesh after the process/scheme that uses it, rather than a name that describes the mesh is not ideal. However, this practice has come about as a result of XIOS iodef.xml files which reference the specific mesh name, having the same mesh names allow for a template iodef.xml to be used. The alternative would significantly increase the number of iodef.xml files (with only a mesh name change between them). Having a constant mesh name, thus results in meshes from separate files having the same name.

This poses a problem for applications like lfric2lfric, where it needs to read different meshes from one or more files, were the meshes might have the same name in each of the files.

The simple solution here is to allow an application to read in the meshes (which have the same name) from individual files as separate instances and allow the application (lfric2lfric) to rename when the instance are created. This allows a method for an application to not violate the unique naming of objects restrictions in the mesh collections. This neatly allows lfric2lfric to use the meshes it requires (with the names they wish) without impacting the upstream applications that produced the input files.

Implementation

Allowing the meshes to be renamed before storing them in the collections is relatively straight forward. An optional argument on the method on the mesh constructors (global/local) allow the mesh objects to be renamed on creation before storing them in the collection. After that, the application should only worked with the "stored mesh names".

Complications

  • Mesh inter-grid maps: These where designed to be valid at the point of mesh generation, i.e. applications should only use maps between meshes sourced from the same file. In this case, the meshes are being renamed and potentially from separate files. The maps themselves are logged from mesh to mesh via their ids, which remain unique, so that shouldn't be an issue. The complication comes when identifying which maps from which files should be assigned to which mesh during setup.

    The solution here is to log at the point of creation the original mesh name and file that the mesh came from. This means that during the intergrid map setup, maps are only read, created and attached if the meshes (using their original names) came from the same file. After the maps are read and attached, the rest of the code should proceed as normal.

  • Chi transforms (Out-of-Scope see Refactor code implementing initial chi transform with respect to North Pole and Schmit stretching #460): This does a lot of unnecessary things, however changes to make it more sensible change kgos. So it should be done as separate PR.

  • Mapping factors: Implementation of mapping factors is too low in the code, in addition it computes for every mesh in the mesh collection which is overkill. It should only need to do this for meshes that are required to be aligned. The routine was overloaded so that a mapping factor could be computed between a global_mesh and a single reference global mesh rather than the whole collection.

Potential downstream issues

  • What mesh name is used for downstream applications? (Out-of-scope) In lfric2lfric's case, multiple meshes may be read in, which may or may or may not be the same. Though what mesh names should be referenced in the output file? original source, original target or something else determined by the application?

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)

Test Suite Results - lfric_core - lfric2lfric_in_and_out/run1

Suite Information

Item Value
Suite Name lfric2lfric_in_and_out/run1
Suite User ricky.wong
Workflow Start 2026-08-21T12:38:16
Groups Run developer
Dependency Reference Main Like
lfric_core mo-rickywong/lfric_core@lfric2lfric_in_and_out 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 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-rickywong Ricky Wong (mo-rickywong) changed the title Lfric2lfric in and out Allow meshes to be renamed on load Aug 20, 2026
Comment thread components/driver/source/mesh/load_global_mesh_mod.f90
Comment thread components/driver/source/mesh/load_global_mesh_mod.f90
Comment thread components/driver/source/mesh/runtime_partition_mod.f90
Comment thread components/driver/source/mesh/runtime_partition_mod.f90
Comment thread components/driver/source/mesh/runtime_partition_mod.f90
Comment thread components/lfric-xios/source/lfric_xios_setup_mod.x90 Outdated
Comment thread infrastructure/source/io/ugrid_2d_mod.F90 Outdated
Comment thread infrastructure/source/mesh/local_mesh_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90
Comment thread infrastructure/source/mesh/panel_decomposition_mod.f90 Outdated
Comment thread infrastructure/source/mesh/partition_mod.F90 Outdated
Comment thread infrastructure/unit-test/function_space/function_space_mod_test.pf
Comment thread infrastructure/unit-test/mesh/global_mesh_mod_test.pf Outdated
@cjohnson-pi

Copy link
Copy Markdown
Contributor

Sci/Tech review: Thanks for making these changes to enable using meshes with the same name in lfric2lfric. I am generally happy with the implementation with only a couple of minor comments. But please could I encourage the developer to move any changes that are not essential to this work to a separate PR - it will make it a lot easier for reviewers to give a proper assessment of the work.

@mo-rickywong

Copy link
Copy Markdown
Contributor Author

All comments addressed, test-suite green, passing back to reviewer

@cjohnson-pi cjohnson-pi 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.

Many thanks for your response to my previous review. I am happy with those changes. There are still just a couple of minor changes to be made, including reverting further changes that are not required for this ticket.

Comment thread components/driver/source/mesh/add_mesh_map_mod.f90 Outdated
Comment thread components/driver/source/mesh/add_mesh_map_mod.f90 Outdated
Comment thread components/driver/source/mesh/runtime_partition_mod.f90 Outdated
Comment thread components/lfric-xios/source/lfric_xios_setup_mod.x90 Outdated
Comment thread infrastructure/source/function_space/function_space_mod.F90 Outdated
Comment thread infrastructure/source/mesh/local_mesh_mod.f90 Outdated
Comment thread infrastructure/source/mesh/partition_mod.F90
Comment thread infrastructure/unit-test/function_space/function_space_mod_test.pf
Comment thread components/driver/source/mesh/add_mesh_map_mod.f90 Outdated
Comment thread components/driver/source/mesh/add_mesh_map_mod.f90 Outdated
@mo-rickywong

Copy link
Copy Markdown
Contributor Author

Back with cjohnson-pi

@cjohnson-pi cjohnson-pi 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.

Thanks for completing the requested changes.

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.

Generally looks okay. I've noted a few issues of code style. If you want to postpone them to a subsequent ticket, please raise the ticket and link to it on this one.

I wont insist on the string changes because I see string handling is shot throughout this change and modifying it would delay further. But consider them for future.

'Adding intergrid map "'// &
trim(source_mesh%get_mesh_name())//'"-->"'// &
trim(target_mesh%get_mesh_name())//'"'
call log_event( log_scratch_space, log_level_info )

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.

Is this an informative or a narrative message? Should it be at log level "info" or "debug"?

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.

Following my reasoning, I'd say debug.

Comment on lines +49 to +53
character(str_max_filename), intent(in) :: input_mesh_file
character(str_def), intent(in) :: mesh_names(:)
character(str_def), optional, intent(in) :: rename_to(:)

character(str_def), allocatable :: names(:)

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.

Is there a reason for not using character(*) and therefore not having to worry about string length?

Are mesh_names and rename_to co-indexed arrays? If so, is there a way to do this so they aren't.

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.

Reason for not using character(*), --- habit. not sure about the co-indexed, Would seem ideally suited for duplicate mesh namelists i.e.

&Mesh
file_prefix=
file_mesh_name=
app_mesh_name=
prepartitioned=
/

with the instance_key_member being the app_mesh_name, but I think that is out of scope on this PR.

if (size(rename_to) == size(mesh_names)) then
deallocate(names)
allocate(names, source=rename_to)
end if

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.

If the arrays are not the same size, rule of least surprise suggests the user should at least be warned of this. It might even be an error condition. At the moment it looks like the rename just doesn't happen, silently.

Furthermore, why do the names have to be the same size?

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.

Good spot on the silent warning. Same size is co-indexing issue if mesh_names is array >1 how do you know which rename goes to which mesh unless you ensure the same size.

type(global_mesh_type) :: global_mesh

if (.not. global_mesh_collection%check_for(mesh_name)) then
character(str_def) :: name

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.

Given that incoming strings could be larger than str_def, you might be better off using character(:), allocatable :: name. Because character arrays are magic, you can still use name = rename_to and it will be automatically allocated to the correct size.

!> @param[in] stencil_depths Depth of cells outside the base cell
!! of stencil for each mesh.
!> @param[in] partitioner_ptr Mesh partitioning strategy
!> @param[in] mapping_factors Partitioning constraints applied to each mesh.

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.

More co-indexing of arrays?

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.

Yup, necessary evil for want factor out code that was committed in the wrong place to begin with. The whole lot needs rethinking, so this is more of a stepping stone


end if ! test if the source has any targets listed

return

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.

No need for return statements at the end of subroutines.

!> @return origin_name Tag name of mesh that identifies it in the
!> UGRID file that it was read in from.
!
function get_origin_name( self ) result ( origin_name )

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.

Is it worth saving 2 characters on get_original_name?

!> @return origin_name Tag name of mesh that identifies it in the
!> UGRID file that it was read in from.
!>
function get_origin_name( self ) result ( origin_name )

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.

Two character saving again.

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.

Like to couch it as a 25% saving

!> @return origin_name Tag name of mesh that identifies it in the
!> UGRID file that it was read in from.
!>
function get_origin_name( self ) result ( origin_name )

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.

Two character saving?

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.

Was more to mean get the name of the mesh in its origin file.

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.

5 participants