-
Notifications
You must be signed in to change notification settings - Fork 39
Allow different meshes to have different maximum halo depths #237
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?
Allow different meshes to have different maximum halo depths #237
Conversation
stevemullerworth
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.
Impacts of this change on the code I own are trivial and acceptable.
jameskent-metoffice
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.
I've left a small comment on aligning the & in one file. I'm happy with everything else, and I'm happy with the results shown in the linked lfric apps ticket. This passes science review.
|
Thanks for the science review. @allynt this is now ready for code review |
mike-hobson
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.
I'm really not a fan of having the stencil_depth() array that has to magically line up with the mesh_names() array, even though there is no connection between the arrays in the design structure. But having discussed the problem at length with AndyC, we can't think of a better way that doesn't involve a refactor of the mesh code - and that would be a different piece of work. So (with just a little reluctance) I'm happy for this to go on.
|
About to have a look at this, though has there been any work in developing this change on the impact if any on the Intergrid maps? I'm assuming that there is none as I don't think (if I recall correctly) that the intergrid maps extend into outer halos. Having had look at this, I'd agree with Mike and AndyC, I don't like the way that the stencil depth has to be the same size as the mesh_names array in one case or has to be an array of 1 in the other. There are some ways to improved the current change, though ultimately it would need a re-assessing and refactoring, so it probably wouldn't be worth the effort. So I wouldn't block this change, (though also reluctantly). Note: This won't work with pre-partitioned meshes as they only take one input for the max stencil depth. I think that there are a few things that would make refactoring easier (though not a small amount of work)
|
You're right, I haven't needed to touch the integrid mesh maps, as these don't extend into outer halos |
mo-rickywong
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.
As code owner, the change is functional, although not ideal. To refactor this area would mean that further suggestions on this change would likely be a waste of effort.
PR Summary
Sci/Tech Reviewer: @jameskent-metoffice
Code Reviewer: @allynt
Our current infrastructure sets a single maximum halo depth for all meshes, irrespective of their relative resolution. This isn't is necessary and doesn't match the use of the meshes. In typical atmospheric configurations, the finest mesh needs a depth appropriate for the transport scheme, which is set by the expected maximum Courant number (e.g. 10). In contrast, the coarser meshes used for multigrid generally only need a halo depth of 2, and yet we still set their depth to 10. For these multigrid meshes, this means the halos can end up extending beyond the adjacent partitions (unnecessarily!)
This is a linked PR, which addresses this by allowing different halo depths to be set for different meshes. The change is to make the driver's
stencil_depthargument an array ofstencil_depths, corresponding to the different meshes.Linked to: MetOffice/lfric_apps#174
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_core - multi_stencil_depths/run3
Suite Information
Task Information
✅ succeeded tasks - 372
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