Enable jedi_lfric to run lfric_atm forecast configurations - #512
Conversation
…cast_config_ifdefphys
…cast_config_ifdefphys
…cast_config_ifdefphys
|
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: |
Retested after merge at head of branch (08cc8b5) and added the log to the description. |
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Hi Steve, just a couple of questions around the metadata
| @@ -1,4 +1,81 @@ | |||
| import=jedi_common/HEAD | |||
| import=lfric-gungho/HEAD | |||
There was a problem hiding this comment.
Why are you looking to change to using gungho directly? As it currently is, jedi-common will import the gungho metadata via lfric-adjoint and lfric-linear. The settings you've added below all look to be included in the jedi-common metadata, so this is duplicating that, which is not ideal.
Is there a specific reason you want to avoid importing lfric-adjoint and lfric-linear? In which case I would suggest we import lfric-gungho directly from jedi_common and move the linear/adjoint imports into the other jedi metadata sections (jedi_tlm_tests, jedi_tlm_forecast_tl, jedi_lfric_tests, jedi_id_tlm_tests and jedi_forecast_pseudo)
There was a problem hiding this comment.
The way the configurations are arranged is not quite right in my view and I would like to refactor things at some point (soon...). In the case of the forecast application, the LFRic Model part of the confirmation should be identical to the lfric_atm configuration. While it is possible that the adjoint configuration contains the same configuration as lfric_atm there is scope for them to conflict I assume. I agree with your comment on moving where the imports are included but Id prefer to postpone that till later as the current setup is works OK.
Id like to see a better setup for the configurations and I dont think we should have jedi_common but I don't have a great background in rose meta-data. I'll explain a little more because Im not sure what is the best way forward...
JEDI configuration background
There are currently four configurations and they include model configurations
forecast: will run the full NL model (lfric-atm) so thelfric_atmconfiguration is usedforecast_pseudo: does not run a model, useslfric-xiosto read a previously run model. We could just include the parts of the configuration that is required, we currently use all of the configuration, its just mostly ignored.tlm_forecast_tl: runs thelinearmodel. This could just use thelinearmodel configuration but as withforecast_pseudowe currently use all of the configuration, the adjoint part of the configuration is ignored.id_tlm_tests and tlm_tests: runs thelinearandadjointmodel to test the correctness of theadjoint(science test: dot product). Theadjointconfiguration which is effectivelylinear+adjointdue to the way the models inherit form each other.
In addition to the outlined configurations. Each of these apps include JEDI emulator specific configuration. This is all stored in a single configuration file which is ok because the JEDI and LFRic configuartions are defiantly distrinct. I would like to have the JEDI and model configurations in separate files at some point as that would make things clearer. In summary, for the forecast app we would have two configurations:
jedi_forecast_configuartion.nml and nl_model_configuration.nml
which would be run via:
$ path/to/bin/jedi_forecast jedi_forecast_configuartion.nml
and jedi_forecast_configuartion.nml points at nl_model_configuration.nml to handle the model construction.
In JEDI the configurations are arranged this way. Todate this has not been an issue issue but we will soon need to build an application with a configuration for the NL (lfric-atm) and TLM (linear+adjoint) models. If we have separate configurations then we would have something like:
jedi_tlm_test_configuartion.nml, nl_model_configuration.nml, tlm_model_configuration.nml with:
$ path/to/bin/jedi_forecast jedi_tlm_configuartion.nml
and jedi_tlm_test_configuartion.nml pointing at nl_model_configuration.nml & tlm_model_configuration.nml with separate jedi objects to handle the NL and TLM model construction.
| @@ -1,18 +1 @@ | |||
| import=jedi_common/vn3.1 | |||
|
|
|||
| #============================================================================== | |||
There was a problem hiding this comment.
This file shouldn't be edited, so please revert these changes
There was a problem hiding this comment.
Reverted the change to this file and committed at: 6601d3d
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
The jedi_common section is there to reduce duplication of the metadata, and so adding duplication here isn't ideal. But I realise this PR has been waiting for a while, so I'll approve and this can be tweaked further down the line.
|
CLA failure due to changing behaviour of checkout action. Bypassing for this PR |
cf5fe4f
into
MetOffice:main
Thanks for reviewing and merging. I'm keen to rationalise the configurations and think I would benefit with some input from you. There will soon be some work aimed at creating multiple model configurations because we need to run and test with a setup that has the configurations for the full NL model (lfric-atm) and the TLAD (linear-adjoint). Ricky did some work on a branch to demonstrate how this might be done in the proof of concept which I will use for the basis of that. So when I have something I would appreciate your input. Thanks, Steve. |
PR Summary
Sci/Tech Reviewer: Matt Shin (@matthewrmshin)
Code Reviewer: James Bruten (@james-bruten-mo)
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: 08cc8b5
Test Suite Results - lfric_apps - LA_512/run11
Suite Information
Task Information
✅ succeeded tasks - 1218
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