Repository navigation
Add additional si3 fields to PostProc app - #73
Pierre Siddall (Pierre-siddall) merged 9 commits into
Conversation
…to CYLC_WORKFLOW_SHARE_DIR at 5330760
github-actions
Bot
added
the
cla-modified
github-actions
Bot
removed
the
cla-modified
There was a problem hiding this comment.
Thanks for this Erica Neininger (@ericaneininger), I think we may have had a chat about why .format is used in moci at some point but I was wondering if these were cases which could be replaced with an f-string to tidy up calls to data type objects?
| if self.rebuild_suffix['REGEX']: | ||
| # Add processor number (if available) to field | ||
| patt = r'.*{}.*({})'.format(match[0], self.rebuild_suffix['REGEX']) | ||
| patt = r'.*{}.*({})'.format(in_field, self.rebuild_suffix['REGEX']) |
There was a problem hiding this comment.
If f-string use can be used here and it doesn't have any impact on how the data is being processed it would be perferable to use it to avoid calls to .format.
There was a problem hiding this comment.
While this is technically true, the performance difference for a single f-string would be near non-existent, and it would not match the style of surrounding code. Therefore I wouldn't call changing just this one (or two) a "tidy up"!
Having said that, I'm happy to stipulate that any new modules should use f-strings, and perhaps - when resource is available - to re-factor the entire app. There are many other hangovers in the app for compatability with Python2.
.format() is prevalent in MOCI simply due to the age of the code and its origins in py2.
| excl_fields = [f.replace('_', '-') for | ||
| f in self.naml.processing.chunking_exclude_fields] | ||
| if excl_fields: | ||
| return r'.*_({})\.nc'.format(('|').join(excl_fields)) |
There was a problem hiding this comment.
Would this also benefit from f-strings?
Pierre Siddall (Pierre-siddall)
left a comment
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
Given the performance speedup would be negligable here, it makes sense from a pragmatic standpoint to allow these changes as proposed. However, future addtions should try to make use of f-strings to help move towards a modern standard of python.
|
I've replaced the deprecated environemnt variable Ive also opened #76, to highlight that there are multiple other instances of deprecated CYLC envrionment variables across the MOCI codebase - fixing them here is well beyond the scope of this PR. |
Pierre Siddall (Pierre-siddall)
left a comment
Pierre Siddall (Pierre-siddall)
left a comment
There was a problem hiding this comment.
Thanks Erica Neininger (@ericaneininger) , this has been retested and now passes. Merging into main now.
f49bf48
into
MetOffice:main
PR Summary
Add additional SI3 fields for processing by PostProc app:
SBC_scalarandicemod_1dCode Reviewer: Pierre Siddall (@Pierre-siddall)
The UKCM-LL configuration being used for CMIP7 production runs is currently using the SRS repo to build MOCI applications. That configuration uses some additional SI3 fieldnames which are not currently processed by the PostProc app.
This PR takes the SRS branch in use in that configuration and updates it for the Git repo.
postproc_2.5 branch (used in u-dv341): https://code.metoffice.gov.uk/trac/moci/log/main/branches/dev/ericaneininger/postproc_2.5_more_si3_fields
Addtionally, the file exclusions from chunking during compression, which are hard wired in the SRS branch, have been upgraded to read from the
&nemo_processingnamelist - hence the addition of rose metadata and an upgrade macro.Code Quality Checklist
(Some checks are automatically carried out via the CI pipeline)
Testing
Coupled_Drivers applicationI've tested this with rose-stem, and also a version of u-dv341 (UKCM configuration) which has been upgraded to use the Git repository.
trac.log
Test Suite Results - moci - TEST_si3/run1
Suite Information
Task Information
✅ succeeded tasks - 99
Security Considerations
Performance Impact
AI Assistance and Attribution
Documentation
Code Review