Skip to content

Inland basin flow coupling#401

Open
Dan Copsey (DanCopsey) wants to merge 25 commits into
MetOffice:mainfrom
DanCopsey:inland_flow
Open

Inland basin flow coupling#401
Dan Copsey (DanCopsey) wants to merge 25 commits into
MetOffice:mainfrom
DanCopsey:inland_flow

Conversation

@DanCopsey
Copy link
Copy Markdown

@DanCopsey Dan Copsey (DanCopsey) commented Mar 26, 2026

PR Summary

Sci/Tech Reviewer: Maggie (@maggiehendry)
Code Reviewer: Erica Neininger (@ericaneininger)

Receive inland basin flow from the rivers executable via OASIS and transfer that water into JULES to end up in the soil.

  • linked MetOffice/jules#89
  • linked MetOffice/um#63

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 Apps rose-stem suite
  • 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_apps - lfric_test_inland_flow/run5

Suite Information

Item Value
Suite Name lfric_test_inland_flow/run5
Suite User dan.copsey
Workflow Start 2026-04-02T16:10:37
Groups Run developer
Dependency Reference Main Like
casim MetOffice/casim@2026.03.2 True
jules DanCopsey/jules@inland_flow True
lfric_apps DanCopsey/lfric_apps@test_inland_flow_v5 False
lfric_core MetOffice/lfric_core@018e40c True
moci MetOffice/moci@2026.03.2 True
SimSys_Scripts MetOffice/SimSys_Scripts@4387949 True
socrates MetOffice/socrates@2026.03.2 True
socrates-spectral MetOffice/socrates-spectral@2026.03.2 True
ukca MetOffice/ukca@2026.03.2 True

Task Information

✅ succeeded tasks - 1165

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

Comment thread science/gungho/source/driver/create_physics_prognostics_mod.F90
Comment thread applications/lfric_atm/metadata/field_def_diags.xml
Comment thread interfaces/jules_interface/rose-meta/jules-lfric/versions.py Outdated
Comment thread interfaces/jules_interface/rose-meta/jules-lfric/versions.py Outdated
Comment thread interfaces/jules_interface/rose-meta/jules-lfric/versions.py Outdated
Comment thread interfaces/jules_interface/rose-meta/jules-lfric/versions.py Outdated
@DanCopsey
Copy link
Copy Markdown
Author

Currently run_lfric_coupled_nwp_gal9-C48_ex1a_cce_fast-debug-64bit fails with:
ERROR: soil_moisture has gone negative with a value of -0.28421709E-13
I will investigate.

@github-actions github-actions Bot added cla-signed The CLA has been signed as part of this PR - added by GA and removed cla-required The CLA has not yet been signed by the author of this PR - added by GA labels Mar 27, 2026
Comment thread interfaces/jules_interface/source/algorithm/jules_extra_alg_mod.x90 Outdated
Comment thread rose-stem/app/lfric_coupled/file/mydef.xml Outdated
@maggiehendry
Copy link
Copy Markdown
Contributor

I'm on leave currently. I'll need to review when I return on 23rd April.

@DanCopsey Dan Copsey (DanCopsey) added this to the Summer 2026 milestone Apr 21, 2026
Comment thread rose-stem/app/lfric_coupled_rivers/rose-app.conf
@maggiehendry
Copy link
Copy Markdown
Contributor

Currently run_lfric_coupled_nwp_gal9-C48_ex1a_cce_fast-debug-64bit fails with: ERROR: soil_moisture has gone negative with a value of -0.28421709E-13 I will investigate.

Did you get to the bottom of this Dan Copsey (@DanCopsey)?

@DanCopsey
Copy link
Copy Markdown
Author

Currently run_lfric_coupled_nwp_gal9-C48_ex1a_cce_fast-debug-64bit fails with: ERROR: soil_moisture has gone negative with a value of -0.28421709E-13 I will investigate.

Did you get to the bottom of this Dan Copsey (Dan Copsey (@DanCopsey))?

I did get to the bottom of it and fixed it. The problem was inland_outflow_rp (and rivers_outflow_rp) was not being initialised to zero (but being initialised to rmdi instead). I fixed it with this change:

https://github.com/MetOffice/jules/commit/a2f80cb9342dddcb464c1909344b6ca5d74626de

If it is a coupled model then it makes it a requirement that these fields are initialised to something (using add_to_list). If they are not sent to add_to_list then they will be initialised with rmdi (so I have corrected the warning message which was issuing the wrong warning).

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.

Macro looks good

…esults even when turned off as it also includes a bug fix which is the multiplying of runoff by flandg in jules_extra_kernel_mod.F90
@DanCopsey
Copy link
Copy Markdown
Author

This code change still changes results in the coupled model even when inland basin flow coupling is turned off as it also includes a bug fix which is the multiplying of runoff by flandg (land fraction) in jules_extra_kernel_mod.F90. I have updated the KGOs in this chanegset:
fd97d92

I think my work here is done. Passing back to Maggie for Sci/tech review.

…ll request MetOffice#505 goes on trunk. MetOffice#505 adds all the other coupling variables but inland basin flow will be missing from MetOffice#505.
Copy link
Copy Markdown
Contributor

@maggiehendry Maggie (maggiehendry) left a comment

Choose a reason for hiding this comment

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

I'm just about finished with your tickets. Just one query in the LFRic apps changes.

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.

Do the changes to the checksums need reverting?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Hi Maggie (@maggiehendry) . The change to the checksums is still needed as there was a bug in the runoff that this branch also fixes. The runoff should be multiplied by the land fraction in order to conserve water. This changes results. See lines 1058 to 1061 of jules_extra_kernel_mod.F90:

https://github.com/MetOffice/lfric_apps/pull/401/changes#diff-f403c1009c2f45aa4edc90a08257ea92cdb90eb7a9b0df7bedd9b8ec91ea3ebf

I will also add this to the ticket description.

Dan.

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.

Oh yes. I remembering seeing the runoff multiplied by land fraction. Thanks.

Copy link
Copy Markdown
Contributor

@maggiehendry Maggie (maggiehendry) left a comment

Choose a reason for hiding this comment

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

Thanks Dan Copsey (@DanCopsey). This is now good to go 😄.

@james-bruten-mo
Copy link
Copy Markdown
Collaborator

Hi Dan Copsey (@DanCopsey)
My PRs #463 and MetOffice/Jules#110 have just been merged which will cause clashes with this. lfric-jules_shared and lfric-jules metadata sections now exist in the Jules repo. The metadata duplication is no longer needed here, so the incoming deletion to jules-hydrology can just be accepted. The incoming deletion of the jules-lfric versions.py file can also be accepted, but the added upgrade macro should be moved to jules-lsm instead.
You shouldn't need any changes on the jules/UM PRs.

@DanCopsey
Copy link
Copy Markdown
Author

Hi James Bruten (@james-bruten-mo) ,
I have updated my LFRic and JULES branches to HOT by merging in main. I moved across the upgrade macro to jules-lsm as you suggested. I then tried running the lfric_coupled rose-stem test to make sure I hadn't broken anything and I had a couple of issues which were easy to resolve. Now the rose-stem test passes and I think you can carry on with your code review.
Thanks,
Dan.

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

Labels

cla-signed The CLA has been signed as part of this PR - added by GA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inland basin flow coupling

5 participants