Skip to content

Safely compute height fields for continuous function spaces#284

Open
tommbendall wants to merge 8 commits intoMetOffice:mainfrom
tommbendall:TBendall/HeightContinuous
Open

Safely compute height fields for continuous function spaces#284
tommbendall wants to merge 8 commits intoMetOffice:mainfrom
tommbendall:TBendall/HeightContinuous

Conversation

@tommbendall
Copy link
Contributor

@tommbendall tommbendall commented Feb 23, 2026

PR Summary

Sci/Tech Reviewer: @jameskent-metoffice
Code Reviewer: @allynt

The sci_get_height_kernel is used to compute the height of DoFs for all function spaces. However, it is not safe for continuous fields (which share DoFs between columns).

The height field has the GH_WRITE and ANY_DISCONTINUOUS_SPACE_1 metadata, which is only appropriate if only one column will ever write to the DoF. However the kernel is written such that both columns write to shared DoFs, which is very dangerous because when using threading, there is no guarantee which thread will write last.

Until now we have not seen an error because neighbouring columns have fortuitously always agreed on the calculation on this kernel. However #238 and LFRic-Apps #179 were the unlucky PRs to expose this. The result was that for threaded tests, KGOs became unreproducible.

This PR fixes that issue, by having separate kernels to compute the height fields for continuous and discontinuous spaces, with appropriate metadata and access patterns for those fields. However this does change KGOs for almost all LFRic-Apps tests.

Details

  • the sci_get_height_kernel has been been renamed to sci_height_discontinuous_kernel (and the corresponding unit-test)
  • the sci_height_discontinuous_kernel has been written k-innermost to run more efficiently
  • a new sci_height_continuous_kernel has been written, to cover continuous spaces such as W2
  • a unit-test for the sci_height_continuous_kernel has been added, with parameters to test both spherical and planar geometries
  • for both height kernels, the config variables (planet_radius, geometry, coord_system) are all passed through the namelist rather than being imported
  • in sci_geometric_constants_mod, the interface to calculate the height fields has changed to pick up the new kernels. Note that:
    • Wtheta is treated as a discontinuous field
    • for continuous fields, the rmultiplicity field needs computing locally
  • extra comments have been added to clarify that the height_w* fields give the height above the Earth's mean radius (and not the height above the surface!)

Linked-To

Blocks

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 - height_continuous_core/run2

Suite Information

Item Value
Suite Name height_continuous_core/run2
Suite User thomas.bendall
Workflow Start 2026-03-09T10:38:22
Groups Run developer
Dependency Reference Main Like
lfric_core tommbendall/lfric_core@TBendall/HeightContinuous False
SimSys_Scripts MetOffice/[email protected] True

Task Information

✅ succeeded tasks - 384

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

@tommbendall tommbendall added bug Something isn't working Linked Apps This PR is linked to a MetOffice/lfric_apps PR labels Mar 9, 2026
@tommbendall tommbendall marked this pull request as ready for review March 10, 2026 09:35
Copy link

@jameskent-metoffice jameskent-metoffice left a comment

Choose a reason for hiding this comment

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

This ticket fixes an issue with getting the height for continuous function spaces. The code is all correct and the new unit test tests both branches of the new kernel.

I've made a few very small comments, including one on a bit of code that hasn't been changed in this ticket, so will leave it to the developer to decide if any changes need to be made. Other than that, I'm happy to approve.

Choose a reason for hiding this comment

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

This isn't part of the code you changed, but looking through this file, should the case (W2H) inventory name be "height_w2h_fe"?

) &
)

case default

Choose a reason for hiding this comment

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

It might be helpful to put a comment here saying that this is for continuous spaces (or continuous horizontal spaces, hence wtheta using the discontinuous kernel)

) &
)

case default

Choose a reason for hiding this comment

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

As above, maybe a comment about continuous spaces

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

Labels

bug Something isn't working Linked Apps This PR is linked to a MetOffice/lfric_apps PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants