Adjust the bdy_lyr local script, and housekeeping#231
Adjust the bdy_lyr local script, and housekeeping#231MetBenjaminWent wants to merge 10 commits intoMetOffice:mainfrom
Conversation
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.
| 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 |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
My only concern with this, and why I opted for a try / except, is when PSyclone runs into an occurrence where a grandchild node doesn't have a name property, it will just crash.
Wrapping it in a try / except protects us from this.
There was a problem hiding this comment.
But the breaking early on a if not none test I'm pro
There was a problem hiding this comment.
I wasn't aware of that. Perhaps adding this in a comment would be a good idea?
There was a problem hiding this comment.
No worries, hopefully this covers it:
4ff21c4
There was a problem hiding this comment.
And for above and below SR advice with try / except preserved:
86496e8
| 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 |
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.
| 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 |
There was a problem hiding this comment.
Same as above
…c_apps into update_part_bdy_psy
|
Changes have been made in line with SR requests, back to SR @oakleybrunt |
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