Adjust the bdy_lyr local script, and housekeeping#231
Open
MetBenjaminWent wants to merge 5 commits intoMetOffice:mainfrom
Open
Adjust the bdy_lyr local script, and housekeeping#231MetBenjaminWent wants to merge 5 commits intoMetOffice:mainfrom
MetBenjaminWent wants to merge 5 commits intoMetOffice:mainfrom
Conversation
This was referenced Feb 11, 2026
oakleybrunt
reviewed
Feb 17, 2026
Contributor
oakleybrunt
left a comment
There was a problem hiding this comment.
Thanks Ben, this looks like a good change with some nice flexibility. Can you have a look over my suggested changes, I think they are slightly clearer than what was proposed.
Comment on lines
+146
to
+158
| found_valid_if = True | ||
| # Often timing handles are placed inside if blocks, | ||
| # check if it is not a known timing call, which should be | ||
| # ignored for spanning a parallel section. | ||
| for routine_grandchild in routine_child.walk(Reference): | ||
| try: | ||
| if str(routine_grandchild.name) in timer_routine_names: | ||
| found_valid_if = False | ||
| except ValueError: # noqa: E722 | ||
| continue | ||
| if found_valid_if: | ||
| start_node = index | ||
| break |
Contributor
There was a problem hiding this comment.
Suggested change
| found_valid_if = True | |
| # Often timing handles are placed inside if blocks, | |
| # check if it is not a known timing call, which should be | |
| # ignored for spanning a parallel section. | |
| for routine_grandchild in routine_child.walk(Reference): | |
| try: | |
| if str(routine_grandchild.name) in timer_routine_names: | |
| found_valid_if = False | |
| except ValueError: # noqa: E722 | |
| continue | |
| if found_valid_if: | |
| start_node = index | |
| break | |
| # Often timing handles are placed inside if blocks, | |
| # check if it is not a known timing call, which should be | |
| # ignored for spanning a parallel section. | |
| for routine_grandchild in routine_child.walk(Reference): | |
| if str(routine_grandchild.name) in timer_routine_names: | |
| # Start node remains None. | |
| start_node = None | |
| break | |
| else: | |
| # Set the start node to the index and keep checking | |
| # grandchildren for calls that invalidate. | |
| # If all are valid, start_node still equals index. | |
| start_node = index | |
| # Now check if we have a satisfactory start node. | |
| if start_node is not None: | |
| break | |
You can break out of this for loop much quicker by using this setup. I have tested this on bl_diags_mod.F90 and the PSyclone generated output is the same.
Comment on lines
+169
to
+179
| found_valid_if = True | ||
| for routine_grandchild in \ | ||
| routine_children[index].walk(Reference): | ||
| try: | ||
| if str(routine_grandchild.name) in timer_routine_names: | ||
| found_valid_if = False | ||
| except ValueError: # noqa: E722 | ||
| continue | ||
| if found_valid_if: | ||
| end_node = index + 1 | ||
| break |
Contributor
There was a problem hiding this comment.
Again, this can be simplified and will be faster than the original. Also tested on bl_diags_mod.F90.
Suggested change
| found_valid_if = True | |
| for routine_grandchild in \ | |
| routine_children[index].walk(Reference): | |
| try: | |
| if str(routine_grandchild.name) in timer_routine_names: | |
| found_valid_if = False | |
| except ValueError: # noqa: E722 | |
| continue | |
| if found_valid_if: | |
| end_node = index + 1 | |
| break | |
| for routine_grandchild in \ | |
| routine_children[index].walk(Reference): | |
| if str(routine_grandchild.name) in timer_routine_names: | |
| # end_node remains None. | |
| end_node = None | |
| break | |
| else: | |
| # Otherwise, set to correct position and continue | |
| # checking for calls that invalidate this. | |
| end_node = index + 1 | |
| # Now check to see if we have a satisfactory end_node | |
| if end_node is not None: | |
| break |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
remove_unspanable_nodes seems too fragile when there are additional timers involved.
Grabbing the first and last sensible node directly seems safer.
This can be swapped out for the parallel transformation coming from STFC hopefully in the future, which may be moved into the global script.
Remove some other parts which are not involved currently in the other scripts.
Also moving bl_diags_mod to the global script, which functionally does the same thing causes crashes.
For saftey, I'm turning this off. The original source OMP is fine for now, we can look at this again later.
Sci/Tech Reviewer: @oakleybrunt
Code Reviewer: @mo-lottieturner
Issue: #255
Umbrella Issue: #106
Code Quality Checklist
Testing
trac.log
Test Suite Results - lfric_apps - update_part_bdy_psy/run10
Suite Information
Task Information
❌ failed tasks - 3
Test Suite Results - lfric_apps - update_part_bdy_psy/run9
Suite Information
Task Information
✅ succeeded tasks - 51
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