-
Notifications
You must be signed in to change notification settings - Fork 49
118 ostia ice ancils #137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
118 ostia ice ancils #137
Conversation
DanCopsey
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this change needed? It does not appear to be related to the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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