-
Notifications
You must be signed in to change notification settings - Fork 24
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 1943 gridstat seeps #2344
Conversation
… SEEPS_MPR lines types.
…or SEEPS_MPR run without error.
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.
There are still problems remaining to be fixed. I inspected the diffs flagged in this GHA run.
It adds 5 new output files and modifies 3 Point-Stat output files.
Please look specifically at this file that is modified:
point_stat_NCMET_NAM_NDAS_SEEPS_360000L_20120410_120000V_seeps.txt
The SEEPS column that previously reported reasonable values between 0 and 1 are now all 0. Also the S, PF, and PV columns that were previously NA are now also 0.
I informed Rachel that our test data and my sample IDL run (lat=(10,20,30,40,50), lon=(20,40,60,80,100)) produce all 0 density_vector with density_radius 0.75. That's why all columns becomes 0 and weighed SEEPS score, too. I will change back to use averaged SEEPS score, not weighted score. |
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.
Inspected the diffs flagged in this GHA run:
- Adds 5 new output files:
ERROR: folder /data/output/met_test_truth missing 5 files
grid_stat/grid_stat_SEEPS_000000L_20211202_000000V.stat
grid_stat/grid_stat_SEEPS_000000L_20211202_000000V_pairs.nc
grid_stat/grid_stat_SEEPS_000000L_20211202_000000V_seeps.txt
stat_analysis_ps/CONFIG_POINT_STAT_filter_seeps.stat
stat_analysis_ps/POINT_STAT_SEEPS.out
It modifies the data in 7 existing output files.
- Diffs in 3 files from Point-Stat:
file1: /data/output/met_test_truth/point_stat/point_stat_NCMET_NAM_NDAS_SEEPS_360000L_20120410_120000V.stat
file1: /data/output/met_test_truth/point_stat/point_stat_NCMET_NAM_NDAS_SEEPS_360000L_20120410_120000V_seeps.txt
file1: /data/output/met_test_truth/point_stat/point_stat_NCMET_NAM_NDAS_SEEPS_360000L_20120410_120000V_seeps_mpr.txt
- For SEEPS_MPR, the P2 column for the same point locations have changed. For example, for
71940 55.18 -118.88
P2 changes from 0.78 to 0.927. Is this expected? - For SEEPS, the TOTAL column generally decreases from 249 to 230. I assume that's because we're doing more filtering and is expected. The MEAN_FCST, MEAN_OBS, and SEEPS columns change, but that makes sense with a different number of points used.
- For SEEPS, the S, PF, and PV columns remain set as a value of 0. Is this expected? If we can populate those columns in the output of Grid-Stat, why aren't they populated for Point-Stat?
- Diffs in 4 files TC-Pairs and TC-Stat that can safely be ignored:
file1: /data/output/met_test_truth/tc_pairs/al092022_20220926_DIAGNOSTICS.tcst
file1: /data/output/met_test_truth/tc_pairs/al132020_CONSENSUS.tcst
file1: /data/output/met_test_truth/tc_stat/DIAGNOSTICS_filter_match_points.tcst
file1: /data/output/met_test_truth/tc_stat/DIAGNOSTICS_stat.out
@hsoh-u I see the details you sent me separately via email. Sorry if I raised issues you already addressed there. Please just confirm that all diffs I noted in my comment above are expected, and I'll approve the PR. |
The differences are expected:
|
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.
After @hsoh-u confirmed that all diffs are expected, I approve of these changes.
I do suspect that testing from @RachelNorth may reveal that some tweaks to the logic are needed, but let's proceed with these changes now.
Thanks for all your work on this @hsoh-u!
Expected Differences
If yes, please describe:
It produces the SEESP text output. It adds SEEPS MPR score, obs_category and forecast category to NetCDF.
If yes, please describe:
SEEPS line type is added like point_stat
Some SEEPS_MPR data is saved to NetCDF output
Pull Request Testing
Added it to unit test
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
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? [Yes or No]
If yes, describe the new output and/or changes to the existing output:
Yes two more unit test: one for grid_stat and the other for stat_ananysisfrom another PR, Feature #2339 stat_analysis_seeps by @JohnHalleyGotway
Pull Request Checklist
See the METplus Workflow for details.
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