Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Testing
@georgemccabe I'm including you on this PR for your code review but also to alert you that the Grid-Diag output variables will change from type int to int64. Note that ncdiff does NOT flag this as a difference.
Also note that I'll be submitting a similar PR to fix this in develop but will include a couple more changes.
Manually tested a run of grid_diag using MET version 10.0.0 that replicates the integer overflow problem. Switched from vectors of type to vectors of type , retested, and confirmed that the overflow problem is gone. Also histograms in the NetCDF output files are now of type ncInt64 instead of int.
Ran unit_grid_diag.xml to confirm that all existing tests run fine.
Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
Review the code changes.
Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]
The existing grid_diag documentation makes no mention of the NetCDF output variable type. So there's no documentation to update.
Do these changes include sufficient testing updates? [No]
I chose not to add a new unit test to exercise integer overflow. Adding a time/compute intensive unit test doesn't seem worth it.
Will this PR result in changes to the test suite? [Yes]
If yes, describe the new output and/or changes to the existing output:
The variable types for all existing grid_diag NetCDF output files will change from "int" to "int64".
Please complete this pull request review by [8/23/2021].
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