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

Enhance Multivariate MODE to change the default merge_flag setting to NONE #2708

Closed
13 of 22 tasks
davidalbo opened this issue Oct 5, 2023 · 3 comments · Fixed by #2713 or #2720
Closed
13 of 22 tasks

Enhance Multivariate MODE to change the default merge_flag setting to NONE #2708

davidalbo opened this issue Oct 5, 2023 · 3 comments · Fixed by #2713 or #2720
Assignees
Labels
component: code cleanup Code cleanup and maintenance issue priority: high High Priority type: enhancement Improve something that it is currently doing
Milestone

Comments

@davidalbo
Copy link
Contributor

davidalbo commented Oct 5, 2023

Describe the Enhancement

Change the merge_flag default to NONE, and allow it to not be set for MvMode fields. If not set right now the code uses a default merge_flag=THRESH and merge_thresh = ">=1.25". MvMode is now implemented so that the user MUST set all the merge_flag (and merge_thresh) values explicitly.

Time Estimate

1 day

Sub-Issues

Consider breaking the enhancement down into sub-issues.

  • See if changing for both mvmode and single var mode causes problems for single var mode.

Relevant Deadlines

November 7 2023 (beta 2)

Funding Source

2702691

Define the Metadata

Assignee

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Labels

  • Review default alert labels
  • Select component(s)
  • Select priority
  • Select requestor(s)

Milestone and Projects

  • Select Milestone as the next official version or Backlog of Development Ideas
  • For the next official version, select the MET-X.Y.Z Development project

Define Related Issue(s)

Consider the impact to the other METplus components.

Enhancement Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.
  • Fork this repository or create a branch of develop.
    Branch name: feature_<Issue Number>_<Description>
  • Complete the development and test your changes.
  • Add/update log messages for easier debugging.
  • Add/update unit tests.
  • Add/update documentation.
  • Push local changes to GitHub.
  • Submit a pull request to merge into develop.
    Pull request: feature <Issue Number> <Description>
  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s) and Development issue
    Select: Milestone as the next official version
    Select: MET-X.Y.Z Development project for development toward the next official release
  • Iterate until the reviewer(s) accept and merge your changes.
  • Delete your fork or branch.
  • Close this issue.
@davidalbo davidalbo added type: enhancement Improve something that it is currently doing component: code cleanup Code cleanup and maintenance issue priority: medium Medium Priority alert: NEED ACCOUNT KEY Need to assign an account key to this issue labels Oct 5, 2023
@davidalbo davidalbo added this to the MET 12.0.0 milestone Oct 5, 2023
@davidalbo davidalbo added priority: high High Priority and removed priority: medium Medium Priority labels Oct 5, 2023
@davidalbo
Copy link
Contributor Author

davidalbo commented Oct 5, 2023

Looks like mode always reads the MODEConfig_default, not the MODEMultivarConfig_default. These default files are similar but not quite the same. The latter does already have default merge_flag=NONE.

mode needs to read an actual config to be able to detect whether it's multivar or not, so on startup it does not know which of these is the default file to read.
One option would be to change the MODEConfig_defauilt so that merge_flag=NONE is the default, which might impact single var mode.
Another option is to read the config as is now to determine if it's multivar or not, then read again using MODEMultivarConfig_default if it was determined it's a multivar case.

@davidalbo
Copy link
Contributor Author

@hertneky @JohnHalleyGotway

I did change the code so that it uses MODEMultivarConfig_default, once it determines the user config has multivar content by doing a 'double read'. If the user config does not have multivar content, MODEConfig_default is used. The former file has merge_flag=NONE as the default, so now if the user does not specify merge_flag for a field, that field will get merge_flag=NONE (unless the user puts something else into the 'parent' part of the config, 'parent' being outside of the individual input fields).

I also added debug printing (level 2) for the case where a field does not have merge_flag/merge_thresh set, and it is defaulting to the parent values, to show what the merge_flag/merge_thresh values that field is using. Just to make sure the user is getting what they want.

I also added some code to disallow certain combinations and error exit:

  1. individual fields with merge_flag specified, but without merge_thresh specified. This is to prevent merge_thresh to come from the parent, which probably would be an error.
  2. individual fields with merge_thresh specified, but without merge_flag. Same idea, to prevent merge_flag to come from the parent, which probably would be an error.

Possible tests to try:

  1. create a config with a field that does not specify merge_flag/merge_thresh and make sure it defaults to NONE.
  2. create a config with a field that has merge_flag but no merge_thresh and observe the error exit.
  3. create a config with a field that has merge_thresh but no merge_flag and observe the error exit.

@davidalbo davidalbo removed the alert: NEED ACCOUNT KEY Need to assign an account key to this issue label Oct 13, 2023
@davidalbo davidalbo linked a pull request Oct 13, 2023 that will close this issue
15 tasks
@davidalbo
Copy link
Contributor Author

The use of the MODEMultivarConfig_default file caused a value to change when Metplus tests were run, it was multivar_name = "ALPHA_BETA_GAMMA". Previously this name was not set so reverted to the default name "Super". Changed the MODEMultivarConfig_default to have multivar_name = "Super".

Another bugfix was put into this branch, found by @hertneky, in which the default output directory was never set for multivar mode, causing the output directory to be the empty string, which caused an error exit. The fix was to set the default output directory to ".", consistent with single var mode.

@davidalbo davidalbo linked a pull request Oct 26, 2023 that will close this issue
15 tasks
@JohnHalleyGotway JohnHalleyGotway changed the title Change merge_flag default in Multivariate Mode to NONE Change merge_flag default in Multivariate MODE to NONE Nov 17, 2023
@JohnHalleyGotway JohnHalleyGotway changed the title Change merge_flag default in Multivariate MODE to NONE Enhance Multivariate MODE to change the default merge_flag setting to NONE Nov 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: code cleanup Code cleanup and maintenance issue priority: high High Priority type: enhancement Improve something that it is currently doing
Projects
Status: 🏁 Done
2 participants