Conversation
tommbendall
left a comment
There was a problem hiding this comment.
The changes to the height field look good.
On the new kernel, I have a question for @MatthewHambley as code reviewer and @TeranIvy who may also have a view: the new copy_field_halo_kernel is just a setval_X which can loop to a variable halo depth. I think this kernel might be more natural as a psykal_light routine -- as a column-based kernel we end up with three nested loops (over columns, dofs-per-cell and then layers) and since we are just copying data up to some specified depth, would it be neater to do this explicitly in some psykal_light code?
However this looks fine if we would be happier keeping this kernel as is
I have no strong preference over how this is done, so am happy with whatever the 3 of you think is best. Just to note that for the purpose I need it, the kernel only gets used on a 2D W3 field, so doesn't actually require any of the nested loops - I only added those in in an effort to make something more generic & flexible! |
I consulted STFC and here's @arporter's reply: "For it to be psykal-lite, we could do with a plan for it to be supported. We recently extended the I would agree with the route that Andy proposed to extend I am happy to open a PSyclone issue for this. I'll leave it to the code reviewer (@MatthewHambley) to decide whether to go down the route of requesting a PSyKAl-lite implementation in this PR, or just a reference to the PSyclone issue in the kernel (either is fine with me personally). |
Thanks @TeranIvy. Aside from the philosophical dislike of the transformation script, I'm not sure this would work anyway in this instance, as the depth into the halo we are looping needs to be passed in from the algorithm, since it is changing with each iteration of the solver. So yes, I think I agree that the best long term solution would be something like an owned_and_halo_dof or setval_halo_X that takes the halo depth argument from the algorithm. Let me know the PSyclone issue number when you have it, and I'll reference in the kernel or PSy-lite code, whichever @MatthewHambley prefers. |


PR Summary
Sci/Tech Reviewer: @tommbendall
Code Reviewer: @MatthewHambley
This PR provides 2 pieces of infrastructure required by the revised PMSL calculation in MetOffice/lfric_apps#348:
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_core - core3.1_pmsl/run1
Suite Information
Task Information
✅ succeeded tasks - 384
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