Repository navigation
Refactor BL use GH Copliot to remove the j index - #742
Benjamin Went (MetBenjaminWent) wants to merge 99 commits into
Conversation
…rides for mphys_kernel_mod to use the correct one
…ns list. Possibly a wider options fix needs to be rolled in with this ticket......
…for now, overrides should be used to move
…dynamic over top k loop
…ions. Depricate old methods, and better align script with existing methods
…tions by accident, needed for raising jules_exp_kernel_mod into local. Also needed pure call option which is a minor extra, and makes the script more flexible
…ic_apps into add_omp_more_kernels
…locally 2D, call to surf_couple_explicit okay with reshape, formatting changes
github-actions
Bot
added
the
cla-modified
|
Benjamin Went (@MetBenjaminWent), what's your planned timing for this PR, noting you've not put a milestone? There are significant changes planned for the Autumn 2026 release, with a lot of new routines coming into the BL directory from the UM under #734, plus #752 [sorry, this PR had a type/brain freeze in the original!] making some minor changes. Would you rerun GH Copilot once vn3.3 is released and look to get this in shortly thereafter? |
Hi Adrian, appologies for not putting a milestone, I was just aiming for the next release, or when the reviews are completed for this PR. |
I've had a quick chat with Chris for how we can manage these together. We also need to think about a Transmute script for the new domain kernel in #734, and do some similar work as to #106 there too. |
iboutle
left a comment
iboutle
left a comment
There was a problem hiding this comment.
Just one comment below.
Regarding merging with other tickets, I think that should be possible as part of the current release
|
|
||
| ! profile fields from level 0 upwards | ||
| real(r_um), dimension(seg_len,1,0:1) :: p_theta_levels, q, qcl, qcf | ||
| real(r_um), dimension(seg_len,0:1) :: p_theta_levels, q, qcl, qcf |
There was a problem hiding this comment.
On line 720 several arrays are declared to be the same size. p_theta_levels is passed to buoy_tq on line 1412. The dummy argument declaration in buoy_tq.h (lines 19-21) is declared of size 0:bl_levels+1 in the last dimension, whereas the remaining dummy args are tdims_l%k_start:bl_levels. all the arrays are declared the same size in this file. Co-Pilot/Claude say this is benign but whilst changing this file it may as well be corrected, so that p_theta_levels is declared ,0:2 , with the other args 0:1 to match the dummy args.
There was a problem hiding this comment.
I wonder if another caller of buoy_tq needs that extra level? I can have a look to see if the +1 can be removed simply.
There was a problem hiding this comment.
I think this is probably a hangover from the UM - the bl_levels+1 index of p is unused in buoy_tq, so it could just be dimensioned as bl_levels there. p_theta_levels passed in from bl_exp_kernel is dimensioned to nlayers, but we will always have bl_levels <= nlayers so this should be okay.
There was a problem hiding this comment.
Cool, thanks for the context Ian, I'll get it removed
There was a problem hiding this comment.
+1 level removed in buoy_tq, testing in prog
| integer(i_um), dimension(seg_len) :: lcbase0, ccb0, cct0 | ||
|
|
||
| ! Temporary 3D buffers (j=1) for legacy PC2 routine interface. | ||
| real(r_um), dimension(seg_len,1,nlayers) :: qcf_latest_pc2, & |
There was a problem hiding this comment.
These 5 arrays are all unused and should be removed.
There was a problem hiding this comment.
qcf_latest_pc2, qcf_earliest_pc2, cff_earliest_pc2, cff_latest_pc2, cf_latest_pc2 lines 419 to 421
There was a problem hiding this comment.
removed
| real(r_um) :: res_factor(row_length, rows) ! dry deposition coeff. | ||
| real(r_um) :: rhokh_1(row_length) ! surface exchange coeff. | ||
| real(r_um) :: res_factor(row_length) ! dry deposition coeff. | ||
| real(r_um) :: f_field(row_length, rows, bl_levels) ! tracer flux from tr_mix |
There was a problem hiding this comment.
array f_field is now unused (spotted by Co-Pilot/Claude) so it could be deleted, or is it replaced by f_field_1D, in which case why just keep f_field and use that?
There was a problem hiding this comment.
Removed duplicate arrays for redundant copies back - updated naming of copy arrays from _1D to _bl
| real(r_um) :: f_field_1D(row_length, bl_levels) ! tracer flux from tr_mix | ||
| real(r_um) :: surf_dep_flux(row_length, rows) ! surf. deposition flux from | ||
| ! tr_mix | ||
| real(r_um) :: surf_dep_flux_1D(row_length) ! surf. deposition flux from |
There was a problem hiding this comment.
Same comment for surf_dep_flux
There was a problem hiding this comment.
Same as above
| @@ -786,7 +784,7 @@ subroutine bdy_expl2 ( & | |||
| ! NTPAR restricted below BL_LEVELS | |||
| i_wt, & | |||
| ! Water tracer loop counter | |||
| il, jl ! LOCAL indexes for land index loops | |||
| il ! LOCAL index for land index loops | |||
|
|
|||
| integer, parameter :: j = 1 ! Array dimension, LFRic Parameter | |||
There was a problem hiding this comment.
I think this declaration for j can be removed?
There was a problem hiding this comment.
Removed, also for other routines where it had persisited, along with it's uses further on.
github-actions
Bot
removed
the
cla-modified
There was a problem hiding this comment.
Claude/Co-Pilot assisted in reviewing these changes, I asked it for a summary, which was useful. I asked if there were any bugs in the changes, it found quite a few problems but not bugs. Some, Ian had already found. It found dead/unused code, and a potential memory issue. I reviewed all these in person and agreed with analysis. These change requests have all been completed. I have scanned the diffs for all 26 files. I spotted one left over integer. paramter :: j = 1 by eye, but Claude had already flagged it! I understand all the changes, have looked at all the code and can pass the PR for Sci/Tech.
NOTE TO REVIEWERS
#630 is on trunk and duplication has been removed
PR Summary
As tasked by Chris, use primarily GitHub Copilot to remove the j index in the boundary layer, otherwise retaining source structure, and KGOs.
Prompts and chat sessions are provided below. An Agent was NOT used.
Changes were done file by file, limiting the AI to changing the declarations first, changing the arrays second, then removing redundant loops like the already completed hand coded changes, updating the member arrays of the types
bl_diagandwtracin the same ways, and rolling these back out where they are used.These were run through the GCC and CCE compilers on the EXs (and AZ), where further changes like where explicit reshapes (copies) where required by routines, or where some implicit reshapes worked.
Finally testing of the OMP developer, and all groups have ensured that these are safe changes to the source.
Also update related Transmute scripts as required.
Sci/Tech Reviewer: Hacka Fett (@christophermaynard)
Code Reviewer: Ed Hone (@EdHone)
Umbrella #106
Issue: #724
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - remove_j_bdy2/run34
Suite Information
Task Information
✅ succeeded tasks - 51
Test Suite Results - lfric_apps - remove_j_bdy2/run26
Suite Information
Task Information
✅ succeeded tasks - 51
Test Suite Results - lfric_apps - remove_j_bdy2/run35
Suite Information
Task Information
✅ succeeded tasks - 1598
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