Enable jedi_lfric to run lfric_atm forecast configurations#512
Enable jedi_lfric to run lfric_atm forecast configurations#512Steven Sandbach (ss421) wants to merge 41 commits into
Conversation
…cast_config_ifdefphys
…cast_config_ifdefphys
…cast_config_ifdefphys
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Code owner (build system) review: A few things seem off. Please explain why the are necessary.
…cast_config_ifdefphys
Im not sure where this is coming from - I have not changed |
|
Matthew Hambley (@MatthewHambley) Ive updated where possible and replied to your review comments above. The changes here is similar to the one applied in #502 and I note your comment about the general build system here: #502 (review) - I assume the same comment applies here? Can you advise if anything else needed? Ricky Wong (@mo-rickywong) - the dependent PR #502 has been merged. I have merged the head of main yesterday and retested via the LFRic and JEDI testing. Please note that there is a file that needs to be copied to $BIGDATA. |
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
It sounds like you've merged another ticket's changes into this branch. Please either strip that off or make sure it's merged up to the head of that branch. Otherwise the merge to trunk will go bad.
This is a branch from #502 as I need the changes from that here. Last week (24th June), I merged in the head of main since the original base branch was committed via #502. According to the GitHub auto merge button there are no conflicts with MetOffice main. There is some of #502 history in my branch but the only way I can get rid of that is to rebase. Id rather not do that as it was quite painful merging up in the first place and I didn't think that was necessary for these PR's as the history will be squashed on commit? |
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Just one outstanding issue.
There was a problem hiding this comment.
Just spotted this on my way past. At 2MiB this is too big for the source code repository. It also seems too big for the "do anything" example. Remember, the examples are there so a new user can prove to themselves that they've compiled an executable which will "do anything." Not one which will "do something."
In light of this, is there a smaller (order a couple of KiB) version of the file which could be used. Can the need to use this file be turned off in the configuration?
There was a problem hiding this comment.
This test case is based on lfric_atm and as far as I can tell there are no other suitable smaller tests. The one that was included previously was based on a gungho configuration: https://github.com/MetOffice/lfric_apps/blob/main/rose-stem/app/gungho_model/opt/rose-app-semi-implicit-for-linear.conf. It is not straight forward to support that configuration in jedi_forecasts because the base configuration has changed significantly so it was removed in favor of a more realistic case (which was being added here as the example case).
There are two options to remove the file-size issue:
- move the files to
$BIGDATAand use a symbolic link to the files. - Revert the change and put back the older configuration - the rose-stem configuration this is based on no longer exists.
In 5cee5ee I have done the later but I would prefer if it is acceptable to do the former (as in add the data and use sym-links). Can you advise on what is appropriate?
Thanks,
Steve
|
Merged up to the head of main via: 79fac20. Retesting via: |
PR Summary
Sci/Tech Reviewer: Matt Shin (@matthewrmshin)
Code Reviewer: Ricky Wong (@mo-rickywong)
This update requires the changes applied in: #502. This branchUpdate forecast config ifdefphyswas created from the branch in that PR:cjohnson-pi/ifdefphys. The change being applied in this PR can be seen via: cjohnson-pi/lfric_apps@ifdefphys...ss421:lfric_apps:update_forecast_config_ifdefphysThe dependent PR #502 is now merged and the branch has been brought up-to the head of main (as of Wed
24/06) and retested via lfric-jedi with PR's included for the update as detailed below.This PR updates
jedi_lfric_teststo enable running of model forecast based on lfric_atm configurations. This requires settingUM_PHYSICSto true. The update largely follows thelfric_atmbuild configurations. I did not:FFLAG_GROUPSbecause I encountered build issues that may be linked to Adjoint code which is included in thejedi_lfriclibrary.A small bug was fixed (removal of clock double tick).
Three new
lfric_atmbased configurations have been added:nwp_gal9-C12,nwp_gal9-C12_daandnwp_gal9-C48_MG. The existinggh-si-for-linear-C12was removed as it was only a placeholder configuration and no longer required.Note: The
C48 testrequires a new input file reference directly at:/home/users/darth/srv/mobb-data/lfric-jedi/lfric-diag/lfric_bg_mi-be607_C48_20210602_la354.nccurrently and used in JEDI testing. Documentation on the file is included in: https://github.com/MetOffice/jjdocs/blob/38d5cfad6fd30cbea67e825775c5a20798490430/docs/Interfaces/ModelInterfaces/Input-Data/lfric-jedi-dir.rst?plain=1#L169The
applications/jedi_lfric_tests/integration-testwas removed as it was problematic when integrating the UM physics and currently not testing anything useful. Furthermore, none of the other applications include integration tests.Note: given the nature of this change I would like to test this branch again before the branch is merged into main.
- is blocked-by #502Code Quality Checklist
Testing
Tested via
lfric-jedi: MetOffice/mo-bundle#1127.trac.log
Retest at: f1ee397
Test Suite Results - lfric_apps - LA_512/run6
Suite Information
Task Information
✅ succeeded tasks - 1206
Previous trac.log
Test Suite Results - lfric_apps - update_forecast_config_ifdefphys/run5
Suite Information
Task Information
✅ succeeded tasks - 1172
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