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.

Sign up for GitHub

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

Jump to bottom

Update develop-ref after #1650 #1652

Merged
merged 5 commits into from
Feb 3, 2021
Merged

Update develop-ref after #1650 #1652

merged 5 commits into from
Feb 3, 2021

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Feb 3, 2021 •
edited
Loading

Pull Request Testing

Note, that I did have to update the logic in mode_conv.pl a couple of times in the develop branch to get the MODE diffing logic working correctly. But it is now.

Here is proof that the diffs are confined to the AREA_RATIO output column from MODE:

grep "ERROR:" /d1/projects/MET/MET_regression/develop/NB20210203/test_regression_20210203.log
ERROR: found     4 differences in row type MODE_POA     column AREA_RATIO    - max abs: 15.43548 
ERROR: failed tests for MODE_POA: num
ERROR: found     5 differences in row type MODE_POA     column AREA_RATIO    - max abs: 59.98333 
ERROR: failed tests for MODE_POA: num
ERROR: found     3 differences in row type MODE_POA     column AREA_RATIO    - max abs: 4.43301 
ERROR: failed tests for MODE_POA: num
ERROR: found    10 differences in row type MODE_POA     column AREA_RATIO    - max abs: 275.6634 
ERROR: failed tests for MODE_POA: num
ERROR: found     5 differences in row type MODE_POA     column AREA_RATIO    - max abs: 6.41113 
ERROR: failed tests for MODE_POA: num
ERROR: found     1 differences in row type MODE_POA     column AREA_RATIO    - max abs: 0.49517 
ERROR: failed tests for MODE_POA: num
ERROR: found     3 differences in row type MODE_POA     column AREA_RATIO    - max abs: 5.62105 
ERROR: failed tests for MODE_POA: num
ERROR: found     8 differences in row type MODE_POA     column AREA_RATIO    - max abs: 275.6634 
ERROR: failed tests for MODE_POA: num
ERROR: found     5 differences in row type MODE_POA     column AREA_RATIO    - max abs: 6.41113 
ERROR: failed tests for MODE_POA: num
ERROR: found     1 differences in row type MODE_POA     column AREA_RATIO    - max abs: 0.49517 
ERROR: failed tests for MODE_POA: num
ERROR: found     5 differences in row type MODE_POA     column AREA_RATIO    - max abs: 6.41113 
ERROR: failed tests for MODE_POA: num
ERROR: found     3 differences in row type MODE_POA     column AREA_RATIO    - max abs: 5.62105 
ERROR: failed tests for MODE_POA: num
ERROR: found     3 differences in row type MODE_POA     column AREA_RATIO    - max abs: 20.41784 
ERROR: failed tests for MODE_POA: num
ERROR: found     3 differences in row type MODE_POA     column AREA_RATIO    - max abs: 6.1697 
ERROR: failed tests for MODE_POA: num
ERROR: found     2 differences in row type MODE_POA     column AREA_RATIO    - max abs: 35.72203 
ERROR: failed tests for MODE_POA: num
ERROR: found    11 differences in row type MODE_POA     column AREA_RATIO    - max abs: 187.9947 
ERROR: failed tests for MODE_POA: num
ERROR: found     4 differences in row type MODE_POA     column AREA_RATIO    - max abs: 70.48582 
ERROR: failed tests for MODE_POA: num
ERROR: found     1 differences in row type MODE_POA     column AREA_RATIO    - max abs: 103.1153 
ERROR: failed tests for MODE_POA: num
ERROR: found     2 differences in row type MODE_POA     column AREA_RATIO    - max abs: 0.42584 
ERROR: failed tests for MODE_POA: num

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

    None needed really. Just confirm the update.

  • Do these changes include sufficient documentation and testing updates? [Yes]
    This PR serves as documentation of the change.

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

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

    As described above.

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), Project(s), and Milestone
  • 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.

JohnHalleyGotway and others added 5 commits January 28, 2021 16:31
@JohnHalleyGotway
Per 1646, one line fix for cut-and-paste error. (#1647)
04e3397
@JohnHalleyGotway
Per #1643, redefine the contents of the existing AREA_RATIO output co…
1186ec3 
…lumn from MODE. Define it as FCST/OBS object area instead of min/max. Update the User's Guide to note the change and also clarify that the MTD VOLUME_RATIO output really is FCST/OBS. (#1650)
@JohnHalleyGotway
Feature 1644 ps_log (#1651)
502c912 
* Per #1644, write rejection reason codes at verbosity 2 when there are 0 matched pairs.

* Per #1644, add a few sentences to Point-Stat, Practical Information chapter about debugging 0 matched pairs.
The mode_conv.pl logic was slightly broken. MET PR #1650 should have …
3e8c422 
…broken the NB but it did not. Turns out the diffing logic is NOT properly distinguishing between single and pair object lines. It does this by looking for an underscore in the OBJECT_ID column. When we added FCST_UNITS and OBS_UNITS, that shifted OBJECT_ID up 2 spots, but the code was still checking the (0-based) 20th column instead of the 22nd. Fixing this now and will rerun NB20210202 to confirm it works again.
The diffing logic for MODE pair lines still was not correct. We'd add…
e1f691c 
…ed the ASPECT_DIFF and CURVATURE_RATIO columns a while ago, but they were missing from the diff logic. This logic really is not good. We need to make it more robust, reading the version-specific header columns from a table file instead of hard-coding them!
@JohnHalleyGotway JohnHalleyGotway added this to the MET 10.0.0 milestone Feb 3, 2021
@JohnHalleyGotway JohnHalleyGotway linked an issue Feb 3, 2021 that may be closed by this pull request
Make Area Ratio calculation consistent (FCST/OBS) #1643
Closed
20 tasks
jprestop
jprestop approved these changes Feb 3, 2021
View reviewed changes
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 confirm the update. Approving.

@JohnHalleyGotway JohnHalleyGotway merged commit 35eadf2 into develop-ref Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jprestop jprestop jprestop approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
MET 10.0.0
Development

Successfully merging this pull request may close these issues.

Make Area Ratio calculation consistent (FCST/OBS)
2 participants
@JohnHalleyGotway @jprestop