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

Bugfix 1932 develop event_equal #1933

Merged
merged 4 commits into from
Oct 5, 2021
Merged

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Oct 5, 2021

Pull Request Testing

  • Describe testing already performed for these changes:

    Ran a full regression test on kiowa in /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1932 to confirm that the new tests I added work as expected and don't produce any unexpected differences.

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

  • Review the code changes.

  • Review the 2 new tc_stat unit test jobs.

  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]

I made no changes to the documentation. Listed below is the description of event_equal from the docs. Note that the description is correct but the code was not behaving as described:

"The event_equal flag specifies whether only those track points common to all models in the dataset should be retained. The event equalization is performed only using cases common to all listed amodel entries. A case is defined by comparing the following columns in the TCST files: BMODEL, BASIN, CYCLONE, INIT, LEAD, VALID. This option may be modified using the -event_equal option within the job command lines."

  • Do these changes include sufficient testing updates? [Yes]

I added 2 new jobs to an existing TC-Stat config file:

  • One job computes TK_ERR summary output for 2 -amodel entries and produces good output -by AMODEL,LEAD.

  • The next job adds a 3rd -amodel name that doesn't actually exist and no output is created, as expected.

  • Will this PR result in changes to the test suite? [Yes]

    If yes, describe the new output and/or changes to the existing output:

    Additional output in the tc_stat/PROBRIRW_stat.out output file. On kiowa, see:
    /d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1932/MET-feature_1932_event_equal/test_output/tc_stat/PROBRIRW_stat.out

  • Please complete this pull request review by [Tuesday 10/5/2021].

Pull Request Checklist

See the METplus Workflow for details.

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

…dling of the LEAD column. We were missing a couple of necessary parentheses.
…ent equalization returns no results when one of the requested AMODEL names does not actually appear in the input dataset.
@JohnHalleyGotway JohnHalleyGotway added this to the MET 10.1.0 milestone Oct 5, 2021
@JohnHalleyGotway JohnHalleyGotway marked this pull request as ready for review October 5, 2021 19:19
…removed for some reason in development since version 10.0.0.
@JohnHalleyGotway JohnHalleyGotway changed the title Feature 1932 event_equal Bugfix 1932 develop event_equal Oct 5, 2021
@JohnHalleyGotway
Copy link
Collaborator Author

Confirming that the only diffs are in the one file with an expected change.
See kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta3/feature_1932/test_regression.log

COMPARING tc_stat/PROBRIRW_stat.out
file1: MET-feature_1932_event_equal/test_output/tc_stat/PROBRIRW_stat.out
file2: MET-develop/test_output/tc_stat/PROBRIRW_stat.out
ERROR: diff error:
59,70d58
< 
< JOB_LIST: -job summary -amodel GPMI -amodel GPMN -line_type PROBRIRW -match_points true -event_equal true -column TK_ERR -by AMODEL -by LEAD -out_alpha 0.05000 
< COL_NAME: COLUMN AMODEL    LEAD TOTAL VALID      MEAN  MEAN_NCL  MEAN_NCU     STDEV      MIN      P10       P25       P50       P75       P90       MAX       IQR     RANGE         SUM TS_INT TS_IND FSP_TOTAL FSP_BEST FSP_TIES FSP
< SUMMARY:  TK_ERR   GPMI 0240000   145   145  63.00708  57.63859  68.37557  32.98283 11.31223 23.1662   36.79723  62.11388  84.77858 108.12667 164.17684  47.98135 152.86461  9136.02687     NA     NA         0        0        0  NA
< SUMMARY:  TK_ERR   GPMI 0480000   242   242 113.10221 105.38428 120.82013  61.25763 12       40.66931  68.01918  99.12658 150.10545 200.47608 318.40382  82.08627 306.40382 27370.73378     NA     NA         0        0        0  NA
< SUMMARY:  TK_ERR   GPMI 0720000   198   198 203.35937 185.82707 220.89166 125.8703  30.51949 82.23762 117.73325 166.9921  246.92702 386.09695 630.05906 129.19377 599.53957 40265.15494     NA     NA         0        0        0  NA
< SUMMARY:  TK_ERR   GPMN 0240000   145   145  45.21925  41.4034   49.0351   23.44376  4.98007 17.27534  24.85743  42.49942  62.14562  78.19647 123.71258  37.28819 118.73251  6556.79147     NA     NA         0        0        0  NA
< SUMMARY:  TK_ERR   GPMN 0480000   242   242  84.40908  78.92438  89.89377  43.53235  5.45977 35.99477  53.42997  76.09949 116.93925 133.75388 218.38121  63.50928 212.92144 20426.99658     NA     NA         0        0        0  NA
< SUMMARY:  TK_ERR   GPMN 0720000   198   198 142.59844 130.77669 154.42018  84.87233 11.19544 58.31118  76.46083 120.13739 187.61197 266.47515 426.85334 111.15114 415.6579  28234.49032     NA     NA         0        0        0  NA
< 
< JOB_LIST: -job summary -amodel GPMI -amodel GPMN -amodel MISSING -line_type PROBRIRW -match_points true -event_equal true -column TK_ERR -by AMODEL -by LEAD -out_alpha 0.05000 
< COL_NAME: COLUMN AMODEL LEAD TOTAL VALID MEAN MEAN_NCL MEAN_NCU STDEV MIN P10 P25 P50 P75 P90 MAX IQR RANGE SUM TS_INT TS_IND FSP_TOTAL FSP_BEST FSP_TIES FSP

Copy link
Collaborator

@jprestop jprestop left a comment

Choose a reason for hiding this comment

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

I reviewed the code changes and the 2 new tc_stat unit test jobs. I compared the additional output in the feature_1932_event_equal tc_stat/PROBRIRW_stat.out output file with the output from develop. It looks like the code is now working as the documentation describes. I approve this pull request.

@JohnHalleyGotway JohnHalleyGotway merged commit 6274fca into develop Oct 5, 2021
@JohnHalleyGotway JohnHalleyGotway deleted the feature_1932_event_equal branch October 5, 2021 21:00
@JohnHalleyGotway JohnHalleyGotway restored the feature_1932_event_equal branch October 5, 2021 21:00
@JohnHalleyGotway JohnHalleyGotway deleted the feature_1932_event_equal branch October 5, 2021 21:00
JohnHalleyGotway added a commit that referenced this pull request Oct 6, 2021
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: John Halley Gotway <[email protected]>
Co-authored-by: Howard Soh <[email protected]>
Co-authored-by: johnhg <[email protected]>
Co-authored-by: Julie.Prestopnik <[email protected]>
Co-authored-by: ericgilleland <[email protected]>
Co-authored-by: George McCabe <[email protected]>
Co-authored-by: John Halley Gotway <[email protected]>
Co-authored-by: jprestop <[email protected]>
Co-authored-by: Julie Prestopnik <[email protected]>
Co-authored-by: hsoh-u <[email protected]>
Co-authored-by: Keith Searight <[email protected]>
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: MET Tools Test Account <[email protected]>
Co-authored-by: lisagoodrich <[email protected]>
Co-authored-by: MET Tools Test Account <[email protected]>
Co-authored-by: j-opatz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix TC-Stat event equalization logic to include any model name requested using -amodel.
2 participants