Profiling: subparameter parser support#353
Conversation
Codecov Report
@@ Coverage Diff @@
## dev/gfdl #353 +/- ##
============================================
- Coverage 38.13% 38.13% -0.01%
============================================
Files 269 269
Lines 75743 75743
Branches 13926 13926
============================================
- Hits 28884 28882 -2
- Misses 41660 41662 +2
Partials 5199 5199 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Hallberg-NOAA
force-pushed
the
prof_subparam_parser
branch
from
d38ba4f to
297994d
Compare
|
This is a relatively simple PR, but it should be reviewed by someone who is proficient in Python. @raphaeldussin or @adcroft, would one of you be willing to take a quick look at it, please, to see if it makes sense to you. This PR has to be handled before we can deal with PR #352. |
raphaeldussin
left a comment
raphaeldussin
left a comment
There was a problem hiding this comment.
I think the function would benefit from a docstring and some comments explaining the logic
| def parse_mom6_param(param_file, header=None): | ||
| params = {} | ||
| for line in param_file: | ||
| param_stmt = line.split('!')[0].strip() |
There was a problem hiding this comment.
param_stmt is a string used as a bool in the logic, how about if line.find("!") == 0
There was a problem hiding this comment.
Thanks for looking over this, @raphaeldussin. My Python may be a bit old fashioned. Is the implicit bool(string) operation frowned upon these days? (I tend to use if var too often, though I had thought this one was safe.)
There was a problem hiding this comment.
FWIW, the PEP8 style guide suggests using if not str: to check if a string is empty:
https://peps.python.org/pep-0008/#programming-recommendations
For sequences, (strings, lists, tuples), use the fact that empty sequences are false:
# Correct:
if not seq:
if seq:
# Wrong:
if len(seq):
if not len(seq):
|
|
||
| # Skip blank lines | ||
| if not param_stmt: | ||
| continue |
There was a problem hiding this comment.
pass is a null operation, whereas continue terminates the loop iteration and begins the next one. Using pass would leave value undefined (or worse, defined from the previous loop) and have unexpected effects.
|
|
||
| # Subparameters | ||
| if param_stmt[-1] == '%': | ||
| key = param_stmt.split('%')[-2] |
There was a problem hiding this comment.
now param_stmt is back to being used as a string
| subheader = key | ||
| if header: | ||
| subheader = header + '%' + subheader | ||
| value = parse_mom6_param(param_file, header=subheader) |
There was a problem hiding this comment.
The function is intentionally recursive, in order to parse the nested p1%p2%p3% ... %p3%p2%p1 blocks. Is there a better solution here?
marshallward
force-pushed
the
prof_subparam_parser
branch
from
297994d to
516c318
Compare
@raphaeldussin Thanks for the suggestions, I've added docstrings and additional comments to explain the recursive behavior of the function. I've also responded to the inline comments. |
The very crude MOM_input parser in the automatic profiler did not support subparameters (e.g. MLE% ... %MLE), which caused an error when trying to read the FMS clock output. This patch adds the support, or at least enough support to avoid errors.
Hallberg-NOAA
force-pushed
the
prof_subparam_parser
branch
from
516c318 to
7f8c6bd
Compare
Hallberg-NOAA
left a comment
Hallberg-NOAA
left a comment
There was a problem hiding this comment.
Thank you for the productive conversation surrounding this PR. It has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/MOM6/-/pipelines/19324.
* add geolat_u, geolat_v, geolon_u, and geolon_v coordinates to the ocean_geometry file.
3 participants
The very crude MOM_input parser in the automatic profiler did not support subparameters (e.g. MLE% ... %MLE), which caused an error when trying to read the FMS clock output.
This patch adds the support, or at least enough support to avoid errors.