Skip to content
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 #1402 config cleanup model applications #1722

Merged
merged 44 commits into from
Aug 3, 2022

Conversation

georgemccabe
Copy link
Collaborator

@georgemccabe georgemccabe commented Jul 27, 2022

This PR involves cleaning up the METplus config files in the repository under parm/use_cases/model_applications.

I also added support for setting match_points in TCPairs wrapper because it was being set via TC_PAIRS_MET_CONFIG_OVERRIDES in a few use cases.

Pull Request Testing

  • Describe testing already performed for these changes:

Ran existing use cases and confirmed there were no differences

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

Ensure all automated tests pass
Review config files to ensure organization of variables is satisfactory and all URLs to documentation are correct

  • 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]

  • Please complete this pull request review by 8/2/2022.

Pull Request Checklist

See the METplus Workflow for details.

  • Add any new Python packages to the METplus Components Python Requirements table.
  • Review the source issue metadata (required labels, projects, and milestone).
  • Complete the PR definition above.
  • Ensure the PR title matches the feature or bugfix branch name.
  • Define the PR metadata, as permissions allow.
    Select: Reviewer(s)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Linked issues with the original issue number.
  • After the PR is approved, merge your changes. If permissions do not allow this, request that the reviewer do the merge.
  • Close the linked issue and delete your feature or bugfix branch from GitHub.

…o documentation. Added old format version of config file that contain many comments with the _DIR/_TEMPLATE variables grouped together for comparison
…use case currently is only available in develop, it will eventually be in latest when the next release is created
…mpty string. Previously a False default value was logged as set to empty string
…or this wrapper is optional and different behavior will be executed if no config file is set
…or this wrapper is optional and different behavior will be executed if no config file is set
…le and replace MET_CONFIG_OVERRIDE variable with TC_PAIRS_MATCH_POINTS
…s done because the value in the TCPairsConfig_wrapped file was TRUE, but the default value in the MET config file is FALSE. Typically if a METplus variable is unset, we do not set any value for the corresponding env var which results in using the value in the default MET config file. Doing this would cause many use cases to change their results unless TC_PAIRS_MATCH_POINTS=True was added to each use case config file. We could do this for our repository, but any use case config files outside of the METplus repo would need to be updated
@georgemccabe georgemccabe added this to the METplus-5.0.0 milestone Jul 27, 2022
@georgemccabe georgemccabe linked an issue Jul 27, 2022 that may be closed by this pull request
21 tasks
# https://metplus.readthedocs.io/en/latest/Users_Guide/wrappers.html#tcpairs
###

TC_PAIRS_INIT_INCLUDE =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these options (e.g. TC_PAIRS_INIT_INCLUDE, TC_PAIRS_VALID_BEG, etc.) be removed if they're just being set to default (aka empty)? That seems to be more in line with how the other use cases are being handled.

@@ -106,15 +146,12 @@ TC_STAT_BASIN =
TC_STAT_CYCLONE =
TC_STAT_STORM_NAME =

# Stratify by init times via a comma-separate list of init times to
# include or exclude. Time format defined as YYYYMMDD_HH or YYYYMMDD_HHmmss
TC_STAT_INIT_BEG =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of the TC_STAT options are empty here as well; seems to be common for the feature relative use cases. Should these be removed for ease of reading, or are they popular/integral to users and should stay?

Copy link
Contributor

@j-opatz j-opatz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left two comments related to the feature relative cases, but they are not necessary changes to my approval of the PR. All of the links that I tested worked, and the rearrangement of the use cases, while a vast undertaking, helps create a natural order to the config files that will hopefully help users dive into these use cases easier.

@hankenstein2 hankenstein2 merged commit a585524 into develop Aug 3, 2022
@hankenstein2 hankenstein2 deleted the feature_1402_config_cleanup_model_app branch August 3, 2022 21:30
@georgemccabe
Copy link
Collaborator Author

@j-opatz , thank you for bringing up the variables with empty values. While going through these changes, I removed many of the empty string values to discover any variables that are required when they should be optional. If any were found, I would update the wrapper to no longer require these variables. It appears I may have missed these few variables, so it would be good to remove them and test to ensure that the same result occurs. I can do this in a separate branch.

This can get tricky, as there are some MET config variables where the default is a list with a single value and an empty list is a valid value for that variable. In this case, the behavior will differ if the METplus variable is unset vs. if it is set to an empty string, so removing the variable would change the results by design. This is unlikely the case for these variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Clean up METplus use case configuration files
3 participants