Repository navigation
Import of high order turbulence closure scheme - #734
Shusuke Nishimoto (mo-snishimoto) wants to merge 13 commits into
Conversation
github-actions
Bot
added
the
cla-modified
Shusuke Nishimoto (mo-snishimoto)
added
enhancement
Thanks for the comment. I edited the PR summary to include it. |
Adrian Lock (Adrian-Lock)
left a comment
Adrian Lock (Adrian-Lock)
left a comment
There was a problem hiding this comment.
First many thanks for the very helpful "namelist_and_diagnostics.pdf" and "code_change_detail.pdf" documentation on the accompanying issue. In addition to my comments on the files, I have two further comments:
I apologise for my most significant proposal, which is to use more meaningful names for the two turbulence schemes than the old UM "9C" and "1A", which unfortunately has quite far reaching consequences. I am very happy to take other suggestions but my proposal is to use "Kprof" and "HoC", respectively.
I like having the "blending" options in the blayer namelist, rather than mixing - as long as everyone else is happy that's good. I'll note explicitly, though, the up-coming clash with my PR #752 where I've added a new blending option
Overall, even if simply porting code from the UM, the interfacing and coupling with the rest of lfric is a very impressive piece of work. Many thanks again.
Adrian Lock (Adrian-Lock)
left a comment
Adrian Lock (Adrian-Lock)
left a comment
There was a problem hiding this comment.
Thank you so much for all that relabeling of the scheme names.
Apologies if I missed it, but it would be good to open an issue to port UMDP 25, that describes this scheme, to the new lfric documentation area. No need to get that port done as part of this PR.
github-actions
Bot
removed
the
cla-modified
|
Thank you so much for looking the code again and the approvement. Though I am prepared for the possibility that further conflict resolution with #714 and #752 might be required depending on the timing of the merge, I also have merged the latest main and resolved conflicts in the following files, heading for merge:
I also modified following unit-tests in the same way as #725 and #752 , (reflected
I confirmed rose-stem (all) suite ends successfully and KGO change for the coupled model disappeared as Ian said above.
Thank you for the suggestion. |
|
Sci/Tech reviewer has approved, so this PR is ready to Code review, Matthew Hambley (@MatthewHambley) . (I am sorry for forgetting to mention. ) |
Matthew Hambley (MatthewHambley)
left a comment
Matthew Hambley (MatthewHambley)
left a comment
There was a problem hiding this comment.
Associated JEDI change does not resolve (404 error).
Does this change close the mentioned ticket? If so, please update accordingly.
The code imported into the "physics schemes" bucket is really unpleasant to read. We afford a bit of latitude to code entering this bucket as it is copied from the UM, so can't be expected to match our code style straight away.
You should raise an issue, and mention it in this one, to refactor that code into a more readable and style compatible form.
| if (bl_scheme == bl_scheme_Kprof) then | ||
| call invoke( setval_c(wvar, 0.0_r_def), & | ||
| setval_c(dsldzm, 0.0_r_def), & | ||
| setval_c(mix_len_bm, 0.0_r_def) ) | ||
| else if (bl_scheme == bl_scheme_HoC) then | ||
| call invoke( setval_c(rhokm_bl, 0.0_r_def) ) | ||
| if (bdy_tke /= bdy_tke_deardorff) then | ||
| call invoke( setval_c(tsq_bl, 0.0_r_def), & | ||
| setval_c(qsq_bl, 0.0_r_def), & | ||
| setval_c(cov_bl, 0.0_r_def) ) | ||
| end if |
There was a problem hiding this comment.
A select case statement may be a better choice here, assuming there will be new schemes in the future. It would also allow an unknown choice to be trapped and reported rather than failing silently.
There was a problem hiding this comment.
Thanks for the comment.
I replaced if statement with case statement.
I also added a case default trap to detect any missing implementation; this ensures that developers trying to add a new BL scheme option in the future will easily notice the misimplementation.
Off-line jules tests where namelist:timestepping=method is 'jules' (e.g. run_jules_canned_test) don't need boundary layer scheme, and bl_scheme is not given in the namelist.
Some other tests such as run_lfric_atm_scm_hd209458b don't use boundary layer scheme ( namelist:section_choice=boundary_layer is 'none' ), and bl_scheme is not given in the namelist similarly.
It is same when namelist:timestepping=method is 'no_timestepping', too.
Therefore this trap is implemented to be active except for those cases (otherwise rose-stem suite fails.).
| l_use_sml_dsc_fixes = l_use_sml_dsc_fixes_in | ||
| l_converge_ga = l_converge_ga_in | ||
| num_sweeps_bflux = num_sweeps_bflux_in | ||
| if (bl_scheme == bl_scheme_Kprof) then |
There was a problem hiding this comment.
It looks like there's no terminal else clause for this test ladder, which means it is possible that nothing will happen. i.e. in the case of an unrecognised option. Is this the desired behaviour?
There was a problem hiding this comment.
I modified the code in the same way as above.
| tke_levels = tke_levels_in | ||
| end if | ||
|
|
||
| ! A negative value for shcu_levels means it should default to tke_levels. |
There was a problem hiding this comment.
Or is it an error which should terminate execution?
There was a problem hiding this comment.
Thank you for pointing out. But I personally would prefer to leave it as it is because the functionality where negative tke_levels is automatically modified to bl_levels is same as UM.
I imagine the reason for the implementation is to use this scheme in many models, combining just 1 common opt/rose-app-3dte.conf .
For example, bl_levels can vary among different configuration such as global-nwp, regional-nwp and so on. But tke_levels is often set to same as bl_levels and must be lower than bl_levels (otherwise, the model will fail.).
In such a case, this functionality is convenient.
|
|
||
| ! Options which are bespoke to the choice of scheme | ||
| select case ( mixing_method ) | ||
| if ( bl_scheme == bl_scheme_Kprof ) then |
There was a problem hiding this comment.
Another decision ladder without default. Is silent fall-through desired behaviour?
There was a problem hiding this comment.
I modified the code in the same way as above.
| logical( l_def ) :: master_length_flag | ||
| logical( l_def ) :: rhogamu_bl_flag | ||
| logical( l_def ) :: rhogamv_bl_flag | ||
| logical( l_def ) :: rhogamt_bl_flag | ||
| logical( l_def ) :: rhogamq_bl_flag | ||
| logical( l_def ) :: tke_shr_prod_flag | ||
| logical( l_def ) :: tke_boy_prod_flag | ||
| logical( l_def ) :: tke_dissp_flag | ||
| logical( l_def ) :: sm25_flag | ||
| logical( l_def ) :: sh25_flag | ||
| logical( l_def ) :: dbdz_flag | ||
| logical( l_def ) :: dvdzm_flag |
There was a problem hiding this comment.
Global variables are to be avoided, particularly when they are being used like this, to pass values by side-effect. Consider what happens when we have two instances of the model in a single executable, such as we might for a combined global/regional application.
The ModelDB object provides a key/value pair store which may be used for this sort of thing. Each model instance has it's own ModeDB so they can never interfere.
There was a problem hiding this comment.
I defined a new derived type to handle this diagnostics flag.
I added an instance of this derived type as local variable in bl_hoc_exp_alg_mod.x90 and passed it to bl_hoc_exp_diags_mod.f90 as an argument.
This diagnostics flag is not needed outside of bl_hoc_exp_alg_mod.x90 , so I implemented like this instead of using key_value_collection_type in modeldb.
There was a problem hiding this comment.
As a general rule, this file should never, ever, be added to. Only removed from.
There was a problem hiding this comment.
I think the routines added in boundary_layer here are unavoidable currently, as they are the imported UM code that we don't want to significantly alter or restructure at this stage to maintain consistency with the UM. I think we also need some more general design of how we are going to handle config variables in the physics codes, i.e. we'll need to pass modeldb through to the physics codes that sit below kernels somehow. It would be better to design and implement this properly rather than try to hack something into this PR.
There was a problem hiding this comment.
I appreciate your comment iboutle .
I removed bl_hoc_diags_mod.f90 and mym_errfunc.f90 from dirtylist.txt because they allow for the removal of global variables without requiring major modifications across numerous source files.
Global variables in files still remaining in dirtylist.txt are:
- a flag indicating whether it is the first call (l_first)
- frequently used coefficients (which are constant during the forecast but can vary depending on the selected option)
- namelist options
Currently, PSyClone cannot pass derived type like config_type to kernels; therefore, to supply all these variables from outside of the kernel, several tens of scalar variables must be passed to bl_hoc_exp_kernel_type as arguments.
And a significant reconstrunction is also needed in subroutines under bl_hoc_exp_kernel_type (i.e. UM source codes), too.
Therefore, as Ian suggested, I would like to first import the existing UM code; once LFRic becomes the mainstream for the development and the infrastructure for passing derived type to kernels is in place, I intend to refactor the code to align with LFRic coding standards (and, of course, eliminate global variables).
There was a problem hiding this comment.
What's going on here. Are we looking at co-indexed arrays, i.e. a set of arrays in which corresponding indexes have some common meaning? If so you should look into multiply-instantiable namelists. That is namelists which may appear multiple times in a namelist file, creating an array of namelist objects in the configurator.
Ask me or Ricky if you need more information.
There was a problem hiding this comment.
This isn't adding anything new, just fixing an app that is wrong & inconsistent with the others - I presume Shusuke needs to do this to allow the upgrade macro to work correctly on all apps.
There was a problem hiding this comment.
Thank you so much for the supplement, iboutle .
Yes, the reason for this change is as Ian said.
The size of the array is originally meant to be the same among
enforce_min_value , equation_form , ffsl_vertical_order ... and so on.
But in the original file, there were some inconsistency (e.g. size of equation_form is 6 though that of enforce_min_value is 1).
This inconsistency doesn't cause any problems such as undefined reference in lfric2lfric because those namelists are not referred in lfric2lfric .
I imagine this inconsistency has been as it is so far due to this.
But I needed to fix this in this PR to allow the upgrade macro to work on all apps (otherwise upgrade macro will fail.).
| if (shcu_buoy) then | ||
| call turbulence_fields%get_field('zhpar_shcu', zhpar_shcu) | ||
| end if | ||
| end if |
There was a problem hiding this comment.
When neither test passes we silently fall through. Is that intended behaviour?
There was a problem hiding this comment.
I didn't make the exact changes suggested in the comment; instead I removed the if statement here entirely. This is because we are merely assiginig an address to the pointer. Accessing a empty field (a variable filled with undefined value) depending on the choice of boundary layer scheme does not cause any issue.
| surface_fields, & | ||
| leonard_klm_tke, leonard_klh_tke, & | ||
| clock) | ||
| end if |
There was a problem hiding this comment.
Another silent fallthrough.
There was a problem hiding this comment.
I replaced if statement with case statement and added a case default trap.
Just to note, enabling the trap based on the timestepping_method is not needed here because slow_physics_alg is not called when timestepping_method is 'jules' or 'no_timestepping'.
github-actions
Bot
added
the
cla-modified
|
Your CLA signature was found on the base branch, but you appear to have modified the CONTRIBUTORS.md file in this PR. Please do not edit the CONTRIBUTORS.md file. If you have already signed the CLA, revert changes to the file and your signature will be picked up. |
|
Thank you for the prompt review and detailed check, Matthew Hambley (@MatthewHambley) . I also apologize for the delay in my response. Currently #742 is under code review, and we would like the PR to be merged into main earlier than this PR because #742 is important for PS49. Thanks
I imagine the 404 Error might be due to the permission problem because as long as I looked into, lfric-jedi is a private repository on the JSCDA-internal GH account.
I opened a new issue #843 to refactor the code and mentioned this PR. |
|
I misunderstood how to use github and failed to send the review request. I apologize for the delay, Matthew Hambley (@MatthewHambley) . Thank you for pointing out, iboutle . |
PR Summary
Sci/Tech Reviewer: Adrian Lock (@Adrian-Lock)
Code Reviewer: Matthew Hambley (@MatthewHambley)
This PR imports high order turbulence closure scheme (3DTE Mk1 scheme) in UM to LFRic.
Please see #46 for details, documentation and extra tests performed.
Linked PRs
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - test_3dte_mk1/run2
Suite Information
Task Information
❌ failed tasks - 2
⌛ waiting tasks - 2
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