118 ostia ice ancils#137
Conversation
github-actions
Bot
added
the
cla-required
github-actions
Bot
added
cla-signed
Dan Copsey (DanCopsey)
left a comment
Dan Copsey (DanCopsey)
left a comment
There was a problem hiding this comment.
I have one change to request relating to missing data indicator in new file and also a couple of comments to reply to.
|
As the code review deadline has passed I am removing this from the Spring 2026 milestone. If there is a problem with this then let me know. Thanks. |
Tim Graham (timgraham-Met)
requested review from
a team,
DrTVockerodtMO,
cjohnson-pi,
iboutle,
James Bruten (james-bruten-mo),
Matt Shin (matthewrmshin),
Steven Sandbach (ss421) and
thomasmelvin
as code owners
Tim Graham (timgraham-Met)
requested review from
Ricky Wong (mo-rickywong)
and removed request for
a team
github-actions
Bot
added
cla-modified
cjohnson-pi
left a comment
cjohnson-pi
left a comment
There was a problem hiding this comment.
Please see #179 (comment) - I think this should give the true diff.
github-actions
Bot
removed
the
cla-modified
Dan Copsey (DanCopsey)
left a comment
Dan Copsey (DanCopsey)
left a comment
There was a problem hiding this comment.
[sci/tech review]
I have gone through all the lines of code that are changed and they all seem sensible. I have looked at the new ancillary file and that looks good too. I have checked that testing has been done with and without the new ancillary file.
I approve sci/tech review and passing it onto Mike Hobson (@mike-hobson) for code review.
DanStoneMO
left a comment
DanStoneMO
left a comment
There was a problem hiding this comment.
Tested JEDI against this and it's all clear, no linked JEDI change will be needed.
Mike Hobson (mike-hobson)
left a comment
Mike Hobson (mike-hobson)
left a comment
There was a problem hiding this comment.
The code changes look fine to me - I've made some very minor comments. There are a few issues on the PR that need resolving:
- You note that you can't think of a way to implement an upgrade macro. I think this is fine and you don't need one. Your changes are in optional configs and the upgrade macros are there for upgrading the main configs. Any jobs using your optional configs are a special case and will be updated manually.
- There is a conversation with Ricky Wong about the way configuration information was being used. I've spoken to Ricky and he says you met and decided to leave the configuration as it was for now. Could you updated the conversation on the PR to note the resolution.
- You note that the
validate_rose_metatask in the test suite is failing. This change can't be merged onto main with a failing test suite. It looks like Ian Boutle has offered a potential solution, have you tried it? - There's a reference in the branch to
/data/users/tim.graham/LFRIC_SEA_ICE_ANCIL/glu_ice. Obviously, we can't havemainreferring to files held in your$DATADIR, so I assume you want this data copied to$BIG_DATA_DIR. Could you provide full paths for where you want the data copied from and to?
There was a problem hiding this comment.
The code for this kernel looks absolutely fine. Perhaps it could benefit from a few blank lines to improve readability. Perhaps after the second contains and maybe separating the declarations from the body of the routine.
There was a problem hiding this comment.
I've added a few extra lines. Hopefully that's better now.
| [namelist:files] | ||
| iau_path='$BIG_DATA_DIR/start_dumps/basic-gal/yak/20210324T0600Z_um2lfric_iau_000001' | ||
| sea_ice_ancil_path='$BIG_DATA_DIR/start_dumps/basic-gal/yak/seaice_ugrid_postqa_fixed' | ||
| sea_ice_ancil_path='/data/users/tim.graham/LFRIC_SEA_ICE_ANCIL/glu_ice' |
There was a problem hiding this comment.
As noted in the overall comments, I'm not sure what to do with this file.
| iau_sst_path='$BIG_DATA_DIR/IAU/Global/um2lfric_sstpert' | ||
| iau_surf_path='$BIG_DATA_DIR/IAU/Global/um2lfric_landda' | ||
| sea_ice_ancil_path='$BIG_DATA_DIR/start_dumps/basic-gal/yak/seaice_ugrid_postqa_fixed' | ||
| sea_ice_ancil_path='/data/users/tim.graham/LFRIC_SEA_ICE_ANCIL/glu_ice' |
There was a problem hiding this comment.
And again, not sure where to put this, more permanently.
| iau_path='$BIG_DATA_DIR/start_dumps/basic-gal/yak/20210324T0600Z_um2lfric_iau_000001' | ||
| iau_pert_path='$BIG_DATA_DIR/IAU/Global/iau_pertinc_start' | ||
| sea_ice_ancil_path='$BIG_DATA_DIR/start_dumps/basic-gal/yak/seaice_ugrid_postqa_fixed' | ||
| sea_ice_ancil_path='/data/users/tim.graham/LFRIC_SEA_ICE_ANCIL/glu_ice' |
There was a problem hiding this comment.
and again
…ity_kernel_mod.F90 Co-authored-by: Mike Hobson <26921912+mike-hobson@users.noreply.github.com>
…_apps into 118_ostia_ice_ancils
Thanks
Done
I've now added a blank upgrade macro and checked that when it is applied, the rose stem scripts group passes successfully. I'll add the log output shortly.
I would suggest that this file and if possible the associated README file are copied to |
|
Mike Hobson (@mike-hobson) Following discussion with Yash, I have updated the global attributes to include: ` |
Mike Hobson (mike-hobson)
left a comment
Mike Hobson (mike-hobson)
left a comment
There was a problem hiding this comment.
Thanks for all those responses, Tim. That's all great, now. The data looks fine and I've passed it by Ben Fitzpatrick (@benfitzpatrick) (as our Information Asset Owner), who says it's fine to go in.
I'm, therefore happy to approve this code review.
Ricky Wong (mo-rickywong)
left a comment
Ricky Wong (mo-rickywong)
left a comment
There was a problem hiding this comment.
Signing off on review, noting config access after next release
10 participants
PR Summary
Sci/Tech Reviewer: Dan Copsey (@DanCopsey)
Code Reviewer: Mike Hobson (@mike-hobson)
This pull request enables sea-ice ancillaries to be read on categories. This is required for surf ancillaries in coupled NWP where we require information about sea-ice at lake points that are not resolved by NEMO. The new functionality is only used for surf ancils at the moment. Adding it for other types of sea-ice ancils would just require a change to xml files but there is no requirement for this at the moment.
To enable the change, I have also had to make some changes to the logic of reading SST and sea-ice ancils. In particular, coupled climate model runs will now need the seaice_source variable set to "start_dump" instead of "ancil" as was used previously. This has no effect on results. I can't see a way to implement an upgrade macro for this change as the required value is different for coupled vs atmosphere only models. Given my team is likely to be the only one upgrading coupled workflows from vn3.0 to vn3.1 I think we will be able to deal with this manually.
The axes of one input file used by two tests have been updated as part of this change.
Code Quality Checklist
Testing
This has been tested in NWP case study workflows at multiple LFRic versions. It has also been tested in AMIP and coupled climate workflows to ensure that it doesn't change results.
The metadata validation task fails at the moment because I have edited metadata at HEAD but the coupled task still points to vn3.0 metadata.
trac.log (with KGO change due to new ancil file)
Test Suite Results - lfric_apps - ice_ancils_apps3p1p1/run6
Suite Information
Task Information
❌ failed tasks - 7
⌛ waiting tasks - 2
trac.log just for scripts group after applying upgrade macro.
Fri 27 Mar 12:55:05 GMT 2026
Git status:
[
""
]
Commit:
33910514725eabba74322dfa38aa1f55e6c6c601
Test Results - Summary
Test Results - Detail
trac.log before changing ancil file
Test Suite Results - lfric_apps - git_ostia_ancils/run1
Suite Information
Task Information
❌ failed tasks - 1
✅ succeeded tasks - 1104
⌛ waiting tasks - 1
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
PSyclone Approval
Sci/Tech Review
(Please alert the code reviewer via a tag when you have approved the SR)
Code Review