linear_model app with UM_PHYSICS compiler flag set to true#502
linear_model app with UM_PHYSICS compiler flag set to true#502cjohnson-pi wants to merge 17 commits into
Conversation
DanStoneMO
left a comment
There was a problem hiding this comment.
Tested this against jedi and all works well. No linked jedi PR will be needed.
Steven Sandbach (ss421)
left a comment
There was a problem hiding this comment.
Thankyou for the update. This change is required by #512 which builds on this branch. Ive added a blocks PR to the summary.
I've added a few comments but happy with the change so Im handing the change over to James Bruten (@james-bruten-mo) for code review.
| @@ -1,15 +1,19 @@ | |||
| ############################################################################## | |||
| # (c) Crown copyright 2023-2024 Met Office. All rights reserved. | |||
There was a problem hiding this comment.
Im not sure what the policy is but I think the start date here should not change.
There was a problem hiding this comment.
The makefile was copied from lfric_atm which has a start date of 2017. Happy to be advised by the code reviewer.
| @@ -0,0 +1,45 @@ | |||
| ############################################################################## | |||
| # (c) Crown copyright 2017-2026 Met Office. All rights reserved. | |||
There was a problem hiding this comment.
If its a new file - should it just be 2026?
There was a problem hiding this comment.
Not sure. It was copied from lfric_atm, which started with 2017. Happy to be advised by code reviewer.
| limit_cfl=.false., | ||
| / | ||
| §ion_choice | ||
| aerosol='none', |
There was a problem hiding this comment.
Are these updated because they have been missed in previous updates?
There was a problem hiding this comment.
Yes given that there is no physics, it seems safer to switch them all to none. In addition this then satisfies the new switch in create_physics_prognostics.
There was a problem hiding this comment.
Thankyou. Is this already applied in the rose configurations?
There was a problem hiding this comment.
Yes. Although some settings are not triggered (so it might have !!aerosol) due to the rose meta validation (which is not applied to the example configuration files).
Steven Sandbach (ss421)
left a comment
There was a problem hiding this comment.
Passing to James Bruten (@james-bruten-mo) for code review.
mo-cjsmith
left a comment
There was a problem hiding this comment.
Initialisation changes look fine.
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
I think this is mostly fine. I was slightly surprised at the number of changes required in gungho. I imagined that any physics initialisation required there was already done for the apps using the physics schemes? Similarly for the new create_physics_model_data subroutine?
The reason for the changes in gungho is that currently when ifdef UM_PHYSICS is turned on, a lot of physics is initialised (which is required for lfric_atm). But we don't want to initialise this for the linear model as the linear model doesn't use physics. So I've tried to rearrange the code slightly so that the linear model doesn't call as much of the physics initialisation, and anything that it does call has an extra switch that can be turned off. Its a bit of a peculiar code change. The linear model doesn't use physics which is why at the moment the UM_PHYSICS is not turned on. But as jedi need the linear and nonlinear models to be run in the same executable, we have to turn UM_PHYSICS on in the compilation. i.e. its turn it on but don't use it. |
James Bruten (james-bruten-mo)
left a comment
There was a problem hiding this comment.
Thanks Christine, that makes sense so happy to approve.
Given this blocks the jedi PR I may ask another SSD member to get this on over the next few days as I'm away for the next week.
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Code owner (buid system) review: A couple of questions regarding build system changes.
| export FFLAGS_UM_PHYSICS = -s real64 | ||
|
|
||
| include $(PROJECT_DIR)/build/fortran/crayftn/$(PROFILE).mk No newline at end of file | ||
| include $(PROJECT_DIR)/../lfric_atm/build/fortran/crayftn/$(PROFILE).mk |
There was a problem hiding this comment.
I don't understand the need for this change. Does it not do exactly the same thing as the original line but in a more complicated manner?
There was a problem hiding this comment.
I'm now calling this lfric_atm file from the linear_model app. So when calling from the linear_model app, PROJECT_DIR=linear_model. But when calling from the lfric_atm app, PROJECT_DIR=lfric_atm. So you are correct that when running lfric_atm it does exactly the same thing.
|
|
||
| $(info UM physics specific compile options) | ||
|
|
||
| include $(LFRIC_ATM_DIR)/build/fortran/$(FORTRAN_COMPILER).mk |
There was a problem hiding this comment.
I don't think there should be a need to do this. What is it achieving?
There was a problem hiding this comment.
Hmm, I'm not exactly sure what it is achieving. My aim is to use the same compiler options as lfric_atm - so I just copied that over. I think that if its missing then there is a failure - although I would need to double check.
PR Summary
Sci/Tech Reviewer: Steven Sandbach (@ss421)
Code Reviewer: James Bruten (@james-bruten-mo)
To allow the linear and nonlinear code to be run in the same executable, the linear_model code needs to be
able to run with UM_PHYSICS set to true. This includes requiring the physics code to be extracted during the build. I propose that given that the linear model will be used in this format operationally, the linear_model app should also be tested in this format.
The main changes are:
The jedi_lfric_tests application has not been updated. I have tried to update this in exactly the same way as the linear_model app and most of the tests pass. However, a few tests fail (e.g. run_jedi_lfric_tests_tlm_tests_nwp_gal9-4OMP-C12_MG_azspice_gnu_fast-debug-64bit). I believe that the reason for this is that the jedi_lfric_tests already have a optimisation/meto-azspice/psykal/global.py that is required for the adjoint code. Therefore it is not possible to simply point to the lfric_atm optimisation. A follow-on ticket will be required to update the jedi_lfric_tests. This will update the makefile so that it is similar to the linear_model app makefile, update the jedi_lfric_tests build and optimisation files so that they are combination of the lfric_atm and adjoint files. Also, the jedi_lfric_tests integration tests will need to be removed as I don't believe it is possible to run these with UM_PHYSICS.
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - ifdef/run42
Suite Information
Task Information
✅ succeeded tasks - 1164
Test Suite Results - lfric_apps - ifdef/run43
Suite Information
Task Information
✅ succeeded tasks - 169
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