-
Notifications
You must be signed in to change notification settings - Fork 38
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
Feature #2586 GenVxMask improvements #2833
Conversation
…d line arguments that can now contain comma-separated lists that should not be split up into separate items
…MASK_OBS_FILE_WINDOW_BEGIN. This just adds support for an additional variation of the config variable names
… files, progress towards #2492. Allow file window range to be specified separately for mask and input files. Other cleanup to move towards consistent wrappers with fewer wrapper-specific overrides of functions like get_command
…low file window range to be specified separately for mask and input files
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 see that all the tests passed for this PR, including the newly added ones, which is great. So there's no issues with the logic. And I looked through the code changes themselves and don't see any obvious issues.
However, I would recommend modifying the naming conventions for the newly added METplus config options. You added options for GEN_VX_MASK_OBS_FILE...
and GEN_VX_MASK_MASK_FILE...
to correspond to the setting of gen_vx_mask -input
and -mask
command line options.
For consistency and clarity, I'd recommend renaming the GEN_VX_MASK_OBS_FILE...
options as GEN_VX_MASK_INPUT_FILE...
throughout since they control the values for the -input
command line option. In the context of gen_vx_mask, there's no distinguishing between forecast and observation. It just process generic data that could come from any source.
…t multiple inputs with labels (used by GridDiag and UserScript wrappers) and typical inputs (all other wrappers). Prior to this change only input templates that have the FCST or OBS identifier were read properly via get_input_templates
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 approve of these changes.
@georgemccabe I see that you made the switch from GEN_VX_MASK_OBS_FILE...
config options to GEN_VX_MASK_INPUT_FILE...
and that all the tests have passed.
Thanks!
Change Summary
-type lat,lon
. See commit 191d858Pull Request Testing
Review code changes and ensure all tests pass (note land_surface:0 will likely report diffs that are unrelated to the changes in this PR)
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]
Do these changes include sufficient testing updates? [Yes]
Will this PR result in changes to the test suite? [No]
If yes, describe the new output and/or changes to the existing output:
Do these changes introduce new SonarQube findings? [No]
If yes, please describe:
It should reduce them!
Pull Request Checklist
See the METplus Workflow for details.
Select: Reviewer(s) and Development issue
Select: Milestone as the version that will include these changes
Select: Coordinated METplus-X.Y Support project for bugfix releases or METplus-Wrappers-X.Y.Z Development project for official releases