Conversation
github-actions
bot
added
the
cla-required
github-actions
bot
added
cla-signed
DanCopsey
left a comment
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.
There was a problem hiding this comment.
Why is this change needed? It does not appear to be related to the issue.
There was a problem hiding this comment.
This was a bug in stable when I branched from it that prevented running of rose stem. The change is due to merging the hot fix on stable into my branch so that I could run rose stem
| [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/seaice_ugrid_postqa_fixed' |
There was a problem hiding this comment.
This file will need to be copied to $BIG_DATA_DIR by the code reviewer. Has Mike been told about this? I have compared these two files:
- /data/users/lfricadmin/data/start_dumps/basic-gal/yak/seaice_ugrid_postqa_fixed.nc (aka $BIG_DATA_DIR/start_dumps/basic-gal/yak/seaice_ugrid_postqa_fixed.nc)
- /data/users/tim.graham/LFRIC_SEA_ICE_ANCIL/seaice_ugrid_postqa_fixed.nc
And the new file has a missing data values of 1e+20 but has sea_ice_area_fraction:_FillValue = -1073741824
This means that when plotted the scale goes up to 1e+20. Please can this file be remade with missing data points renumbered to -1073741824 (-32768.0*32768.0) which is the standard missing data indicator used by LFRic and JULES.
There was a problem hiding this comment.
Good spot Dan. I created these files by modifying the axes of the originals so that results didn't change. However, given that this has changed the missing data indicators in this way perhaps it would be better to properly generate new files using the new ugants code developed by Rhiannon. This will change results but may be more robust in the long term.
There was a problem hiding this comment.
I agree that it would be good to regenerate these files properly with the correct missing data indicator. Once this is done then let me know and I think this is the only thing holding me back from passing sci/tech review.
There was a problem hiding this comment.
You could possibly use an nc-tool to change the MDI values back again?
|
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. |
timgraham-Met
requested review from
a team,
DrTVockerodtMO,
cjohnson-pi,
iboutle,
james-bruten-mo,
matthewrmshin,
ss421 and
thomasmelvin
as code owners
timgraham-Met
requested review from
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
…_apps into 118_ostia_ice_ancils
iboutle
left a comment
iboutle
left a comment
There was a problem hiding this comment.
On the failing metadata check - I believe the required way of handling this is to have a blank upgrade macro, which when run will force the apps to pick up the head metadata instead of the vn3.1 metadata.
Looks fine to me though.
cjohnson-pi
left a comment
cjohnson-pi
left a comment
There was a problem hiding this comment.
I don't think this actually affects any of the linear or regional model code directly. So approved regarding this.
There was a problem hiding this comment.
I know this is draft, but comment is a heads up.
General comment on the gungho/**/driver routines, though I would suggest it applies all the files. Access to module scope configuration variables has been reworked.
#225 is in code review to remove the intermediate access pattern, however, there is nothing to prevent developers from accessing from modeldb%config now. Adding/accessing new namelists variables from module scope only adds to the work that will need to be taken out.
Documentation on the new api can be found here.
https://metoffice.github.io/lfric_core/how_to_use_it/configuration/using_configuration.html#using-the-config-object
If a routine has access to modeldb then it can already access all namelists via modeldb%config. If not, consider passing modeldb%config down (if modeldb is not required) as there is less chance of cyclic dependencies occurring at a later date.
| help=The sea ice fraction (and thickness if amip_ice_thick is false) can either be read from an static ancillary file | ||
| = (typical in climate runs or NWP forecasts using an analysis file), or from a SURF derived ancillary file | ||
| = To use a SURF produced ancillary-style file, select `surf` else just `ancillary` | ||
| = | ||
| =The iodef file must be consistent with this setting, specifying | ||
| = seaice from either a static ancillary file or a SURF-derived ancillary file. | ||
| sort-key=01a |
There was a problem hiding this comment.
No reference to the new start dump option, any caveats on the start dump i.e. where it is sourced? formatting? etc.
| sst_source_start_dump, & | ||
| sea_ice_source, & |
There was a problem hiding this comment.
| sst_source_start_dump, & | |
| sea_ice_source, & | |
| sst_source_start_dump, & | |
| sea_ice_source, & |
& out of alignment
@mo-rickywong I've had a go at modifying this in the way I think you mean but I must be doing something wrong as compilation is failing. If you are in next week, it would be useful if you could have a look what I'm doing wrong. |
7 participants
PR Summary
Sci/Tech Reviewer: @DanCopsey
Code Reviewer: @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
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