Gregory-Rowntree convection - PSyclone optimisation and conversion from CELL_COLUMN to DOMAIN kernel #99
Conversation
|
Note this PR is a replica of the ticket 788 on MOSRS. Please see previous review comments and CO comments there. |
Sci/Tech Review
Please alert the code reviewer via a tag when you have approved the SR Apart from the CLA signing, ticket is in the same status as when I approved it before in the track ticket 788. CO approval also still stands. CO further development requests are agreed to be handled in a follow up issue which should be linked with this PR and original Issue. Approved pending CLA signing, and new issue for CO requests from trac788. |
github-actions
bot
added
cla-signed
ericaneininger
left a comment
ericaneininger
left a comment
There was a problem hiding this comment.
In general, I'm happy with this change.
There are some code styling niggles - comments scattered where appropriate.
| """ | ||
|
|
||
| from psyclone_tools import redundant_computation_setval, colour_loops, view_transformed_schedule |
There was a problem hiding this comment.
Please stick to a line length of 80char.
| ############################################################################## | ||
| # Copyright (c) 2025, Met Office, on behalf of HMSO and Queen's Printer | ||
| # For further details please refer to the file LICENCE.original which you | ||
| # should have received as part of this distribution. | ||
| ############################################################################## | ||
|
|
||
|
|
There was a problem hiding this comment.
Please use current Copyright header:
| ############################################################################## | |
| # Copyright (c) 2025, Met Office, on behalf of HMSO and Queen's Printer | |
| # For further details please refer to the file LICENCE.original which you | |
| # should have received as part of this distribution. | |
| ############################################################################## | |
| # ----------------------------------------------------------------------------- | |
| # (C) Crown copyright Met Office. All rights reserved. | |
| # The file LICENCE, distributed with this code, contains details of the terms | |
| # under which the code may be used. | |
| # ----------------------------------------------------------------------------- |
There was a problem hiding this comment.
Please follow the Momentum template for copyright header: https://github.com/MetOffice/Momentum/blob/main/docs/COPYRIGHT_TEMPLATE
There was a problem hiding this comment.
yes sorry - I did give the correct header everywhere else! NOw corrected here.
| """ | ||
|
|
||
| from psyclone_tools import redundant_computation_setval, colour_loops, view_transformed_schedule |
There was a problem hiding this comment.
We plan to deploy ruff and fortitude for Python and Fortran lint/format, and line-length limits will be 88 and 132 (max) characters, respectively.
| # The file LICENCE, distributed with this code, contains details of the terms | ||
| # under which the code may be used. | ||
| ############################################################################## | ||
|
|
There was a problem hiding this comment.
Please add a description comment for this file.
Working practices require adherence to PEP8 standards please. There are some issues with line length in this file. ruff/black are recommended tools for checking.
|
|
||
| [namelist:physics=conv_gr_segment] | ||
| compulsory=true | ||
| description=Number of segments to be processed in gregory rowntree convection |
There was a problem hiding this comment.
| description=Number of segments to be processed in gregory rowntree convection | |
| description=Number of segments to be processed in Gregory-Rowntree convection |
| @@ -1129,12 +1132,12 @@ subroutine conv_gr_code(nlayers, & | |||
| delta_smag, tnuc_nlcl_um | |||
|
|
|||
| ! single level integer fields | |||
| integer(i_um), dimension(row_length,rows) :: ntml, ntpar, lcbase, & | |||
| integer(i_um), dimension(ncells,1) :: ntml, ntpar, lcbase, & | |||
There was a problem hiding this comment.
Please keep & in line to match surrounding code
| it_lcbase,it_lctop, it_ccb, it_cct, it_ccb0, it_cct0, it_kterm_deep,& | ||
| it_kterm_shall, it_cg_term, it_lcbase0, freeze_lev, ccb, cct, lctop | ||
|
|
||
| ! single level logical fields | ||
| logical, dimension(row_length,rows) :: land_sea_mask, cumulus, & | ||
| logical, dimension(ncells,1) :: land_sea_mask, cumulus, & |
There was a problem hiding this comment.
Please keep & in line to match surrounding code
|
|
||
| real(r_um), dimension(row_length,rows) :: zlcl, t1_sd, q1_sd, w_max, & | ||
| real(r_um), dimension(ncells,1) :: zlcl, t1_sd, q1_sd, w_max, & |
There was a problem hiding this comment.
Please keep & in line to match surrounding code
|
|
||
| ! Water tracer fields which are not currently used but are required by | ||
| ! UM routine | ||
| real(r_um), dimension(1,1,1) :: q_wtrac, qcl_wtrac, qcf_wtrac, & | ||
| real(r_um), dimension(ncells,nlayers,n_wtrac) :: q_wtrac, qcl_wtrac, qcf_wtrac, & |
There was a problem hiding this comment.
Please keep & in line to match surrounding code
| conv_snow_3d(map_wth(1) + k) = conv_snow_3d(map_wth(1) + k) + & | ||
| it_conv_snow_3d(1,1,k) * & | ||
| one_over_conv_calls | ||
| do i = 1, ncells |
There was a problem hiding this comment.
The recommendation for continuation characters is (a) to match the surrounding style - or alternatively (b) to remove whitespace between the end of the code and the &.
Most of the surrounding code appears to vertically align &s for code blocks.
4 participants
PR Summary
Sci/Tech Reviewer: @MetBenjaminWent
Code Reviewer: @ericaneininger
Code Quality Checklist
(Some checks are automatically carried out via the CI pipeline)
style guidelines
readability of the code
Testing
acceptable (eg. kgo changes)
tests, unit tests, etc.)
and have tests been allocated to an appropriate testing group (i.e. the
developer tests are for jobs which use a small amount of compute resource
and complete in a matter of minutes)
trac.log
Security Considerations
Performance Impact
performance measurements have been conducted
AI Assistance and Attribution
of Generative AI tool name (e.g., Met Office Github Copilot Enterprise,
Github Copilot Personal, ChatGPT GPT-4, etc) and I have followed the
Simulation Systems AI policy
(including attribution labels)
Documentation
confirmed that it builds correctly
PSyclone Approval
inteface, optimisation scripts, LFRic data structure code) then please
contact the
tooscollabdevteam@metoffice.gov.uk
Sci/Tech Review
Please alert the code reviewer via a tag when you have approved the SR
Code Review