-
Notifications
You must be signed in to change notification settings - Fork 58
Stochastic Physics Fixes #148
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?
Conversation
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.
Hi Tom,
I have started to try to find my way about reviewing the LFRIC ticket. I have got some preliminary questions:
- Do we have a diagram of LFRIC staggered vertical grid to understand why SKEB levels need to be subtracted by one and added the offset for the vertical phasing?
- Does the doxygen documentation include variable arguments, if that is the case, does it pick up the change in the "stph_fp_main_alg" argument when including "level_offset"?
- Which tasks of the all test in stoch_phys_fixed/run4 include stochastic physics? Is there a twin run without it where timings can be compared?
Thanks!
|
Thanks Claudio. Here are my attempts to answer your questions:
Good question. I have been back through the UM and LFRic implementations to triple-check this. The context is that SKEB needs the forcing pattern at p-levels while SPT needs it on T-levels. The UM calculates a The UM's arrays have a vertical structure in which T-levels are indexed from 0 to N, while p-levels are indexed from 1 to N. Whereas in LFRic, fields are a single 1D array using a Dofmap, and are written to with a pattern like: So because this indexing is different, the solution Ian proposed was to offset the forcing pattern used in SKEB (calculated in W3) so that it creates a field matching that in the UM.
Yes the doxygen documentation will pick up this description of the new argument: https://github.com/MetOffice/lfric_apps/pull/148/changes#diff-15bab3cd211a7d31b56b0fc280bd500d9cc2f6622bb07f31de75a5d503333cebR50
All of the Mean time for
My interpretation is that the computational impact of the changes in this branch are smaller than general variability for these tests (which is what we expect!) |
|
Thanks Tom for your detailed explanation! I think I finally understood the need for "level_offset". The UM uses the same defition of "kr" for the vertical phase shifting factor, both defined in eta_theta_levels at SKEB at stph_skeb2-stph_skeb2.F90#L1351 and SPT at for_pattern.F90#L459 and copied below, I would think that it is not very consistent (SKEB should prob. use rho_levels instead of theta_levels).
My original implementation in LFRIC was also applied at theta levels for both, SKEB and SPT, at stph_fp_main_alg_mod.x90#L163, shown below.
Adding the "level_offset" is probably the most efficient solution, although something like the code below may be clearer, and not hugely more expensive (the vertical loop should be less than 50-100 iterations!). Note, the name for "get_level_type" is probably different. It is probably worth defining the vertical space of SKEB and SPT bottom and upper levels in their help metadata at rose-meta.conf. |
|
Thanks, I think the extra commented lines explains well the purpose of level_offset. I have no other comments or suggestions, passing the ticket to code review.
|
mo-claudiosanchez
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.
Thanks, the purpose of level_offset is now quite clear thanks to the extra comment lines.
Passing ticket for code review.
|
@oakleybrunt this is ready for code review, but I just wanted to make you aware this is linked to MetOffice/lfric_core#222 which is has been assigned to @cameronbateman-mo |
Sorry Tom, reassigned to Cameron for both now |
cameronbateman-mo
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.
All looks good, moving to approved.
PR Summary
Sci/Tech Reviewer: @mo-claudiosanchez
Code Reviewer: @cameronbateman-mo
This ticket applies a handful of simple fixes to issues in stochastic physics:
level_offsetargument tostph_fp_main_algCode Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - stoch_phys_fixes/run4
Suite Information
Task Information
✅ succeeded tasks - 1456
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