Conversation
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Driver code owner review: Simple changes to what's in and what's out.
|
This looks good to me so far... I think I mentioned this before, but the update of the fields in fast physics could be further simplified? As it stands The biggest win would be to do this for the convection increments to What do you reckon? Or is this best done in a separate PR? Cheers! |
Yep, I agree with the plan but would like to tackle that as a separate PR - this one seems big enough, and I can still just about convince myself that the KGOs are unchanged for several jobs. Untangling the issues around convection & PC2 increments will lead to more changes because of the way things are added and subtracted, and I think we'd want to change the convection increment from T to theta at the same time. Once that is done I think we could get each algorithm to update the latest fields, but currently the convection update can't happen in the convection algorithm because it needs to happen after the PC2 routine. |
Yep no problem at all to leave this for a later PR; I'm happy to tidy the convection / PC2 increment handling up at some point... Cheers! |
|
Hi just checking I understand what's going on with the refactoring of boundary-layer diagnostics code? So, previously there was a call to However, Is it a case of these diagnostics could have still been output inside bl_imp_alg_mod.x90, but splitting-up the code in Cheers! |
No - the diagnostics output by bl_extra_diags are things like visibility, which require both the convection diagnostics and BL diagnostics as input. Therefore the plumbing of things like t1p5m is required because these are input to the fog and vis calculations, which also require the convective rain and suchlike. It was kind of surprising, but also somewhat happy, that just moving the kernel as-is was actually what was required! |
Ah OK yes that makes sense; so |
MichaelWhitall
left a comment
There was a problem hiding this comment.
Ah I just found the "submit review" button! I think its been hiding all my in-line comments from you until I press this button... Can you see the comments now?
science/gungho/source/algorithm/physics/fast_physics_alg_mod.X90
Outdated
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/diagnostics/bl_imp_diags_mod.x90
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/diagnostics/bl_imp_diags_mod.x90
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/algorithm/bl_imp_alg_mod.x90
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/diagnostics/conv_diags_mod.x90
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/kernel/bl_imp_kernel_mod.F90
Outdated
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/kernel/bl_imp_du_kernel_mod.F90
Outdated
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/algorithm/conv_gr_alg_mod.x90
Outdated
Show resolved
Hide resolved
interfaces/physics_schemes_interface/source/algorithm/conv_gr_alg_mod.x90
Show resolved
Hide resolved
science/gungho/source/algorithm/timestepping/semi_implicit_timestep_alg_mod.X90
Show resolved
Hide resolved
Hopefully I've addressed all of these... |
Yep this all looks good to me :) The bot seems to've already automatically moved this out of sci/tech review for me (I assumed I'd need to press a button somewhere to sign this off?) Maybe it counts as signed-off once all the comments are marked as "resolved"?) Cheers! |
MichaelWhitall
left a comment
There was a problem hiding this comment.
Oh no wait there actually is an "approve" button in here, which I hadn't pressed yet; I think the bot's activity was a bit premature? Anyhow I'm pressing the button now ;)
The bot moved it to "changes requested" - I should have moved it back to sci/tech after doing them. I think you need to tick the boxes in the science review section, and then tag the code reviewer in a comment saying your happy it's passed - the bot is supposed to move it to code review, although I've never actually got that to work! |
|
Sci/Tech review approved on this one, passing over to Lottie Turner (@mo-lottieturner) for code review... Cheers! |
PR Summary
Sci/Tech Reviewer: MichaelWhitall
Code Reviewer: Lottie Turner (@mo-lottieturner)
This PR refactors fast physics to enable 2 key pieces of science infrastructure (which are also implemented by this PR):
To enable this, it has been necessary to properly separate the boundary layer and convection increments, which were previously badly inter-mixed. Fast physics now tracks "latest" fields which are updated by each scheme in turn (since the physics here is sequential by design), and then finally constructs an increment from the difference between the final and initial fields. Because this changes the calculation order, it changes many KGOs for lfric_atm
The calls to these schemes in slow physics have also had to be updated to be consistent, and diagnostics previously produced in bl_imp_alg but that depend on convection have had to be separated into their own algorithm to be called after BL and convection have definitely both been done.
Ideally, the new "BL before conv" functionality would be switched on in the comorph_dev tests, however these currently fail when doing so. I will investigate this separately, as the failure is not connected to the code introduced by this PR (particularly the refactoring, which we want to consolidate back to main asap), but is a feedback on model evolution. Doing so would also conflict with several other PRs in flight (#292, #326) so is best left as a separate PR.
closes #228
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - fast_refactor/run1
Suite Information
Task Information
✅ succeeded tasks - 1310
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