Closed JohnHalleyGotway closed 4 months ago
@JohnHalleyGotway thanks for making this fix, I will test it shortly.
Regarding asking for additional debugging that would have helped, I think it's worthwhile to consider adding a high DEBUG
option (10? 10+?) to print the "header" (an entire row of the 11-column data) for each of these rejection types:
https://github.com/dtcenter/MET/blob/16aac923a8db931cf99e6a24ed6ae275b4a664d8/src/libcode/vx_statistics/pair_data_point.cc#L424-L430
In debugging I actually added some print statements here: https://github.com/dtcenter/MET/blob/16aac923a8db931cf99e6a24ed6ae275b4a664d8/src/libcode/vx_statistics/pair_data_point.cc#L1020-L1022
So that would be one place where if rej_vld
is being incremented, check if the DEBUG
exceeds some level and if so dump the row of data that's being rejected. I do think this would not be used very frequently, but the Python embedding case is a good example of where we were confident about this section of code (in pair_data_point.cc
), but then came along with another way to serve data to this class (Python embedding). Future vehicles for serving data to MET that are yet to be defined would benefit from this added debugging.
Note that this bug in Point-Stat and Ensemble-Stat occurs when reading point observations through Python embedding. If some of the obs share a common valid time, then there is a mismatch in the unique list of numeric valid times and the non-unique list of valid time strings.
Expected Differences
[x] Do these changes introduce new tools, command line arguments, or configuration file options? [No] If yes, please describe:
[x] Do these changes modify the structure of existing or add new output data types (e.g. statistic line types or NetCDF variables)? [No] If yes, please describe:
Pull Request Testing
[x] Describe testing already performed for these changes: Please see the testing described in the body of #2897. I demonstrated the problem using the nightly build versions of MET and the solution using
seneca:/d1/projects/MET/MET_pull_requests/met-12.0.0/beta5/MET-bugfix_2897_develop_python_valid_time
. I did also test with all 151 obs for the TCI use case and confirmed that the consistent and correct valid time is now being used.[x] Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions: Review the changes and test using
seneca:/d1/projects/MET/MET_pull_requests/met-12.0.0/beta5/MET-bugfix_2897_develop_python_valid_time
as you see fit.[x] Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No] None needed.
[x] Do these changes include sufficient testing updates? [No] None needed as these will impact the output from METplus use cases and it will be tested there.
[x] Will this PR result in changes to the MET test suite? [No] If yes, describe the new output and/or changes to the existing output: I do not expect diffs from MET.
[x] Will this PR result in changes to existing METplus Use Cases? [Yes] If yes, create a new Update Truth METplus issue to describe them. I expect modified output from the TCI use case and it has the potential to impact output from other use cases.
[x] Do these changes introduce new SonarQube findings? [No] If yes, please describe: I hope not.
[x] Please complete this pull request review by [Thurs 5/23/24].
Pull Request Checklist
See the METplus Workflow for details.