-
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 1844 python embedding #2010
Conversation
…e moved to the base class MetPointObsData
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.
Running a full regression test in kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta5/feature_1844 reveals unexpected differences. Specifically in the files listed below:
egrep -i "ERROR:|file1:" test_regression.log | egrep -B 2 ERROR: | grep ensemble | grep file1
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_NCEP_1.0DEG_20120410_120000V_ecnt.txt
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_NCEP_1.0DEG_20120410_120000V_orank.nc
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_NCEP_1.0DEG_20120410_120000V_orank.txt
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_NCEP_1.0DEG_20120410_120000V.stat
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_ONE_CDF_BIN_20120410_120000V_ecnt.txt
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_ONE_CDF_BIN_20120410_120000V_ens.nc
file1: MET-feature_1844_python_embedding/test_output/climatology/ensemble_stat_ONE_CDF_BIN_20120410_120000V.stat
These differences require further investigation. No explanation for why they should differ is listed in the pull request.
Since we're otherwise ready to do the beta5 release, I recommend we do the following:
- Proceed with the beta5 release on 1/14/22.
- Move issue MET#1884 and this PR MET#2010 over to the beta6 project.
- After completing the beta5 release, merge the latest from develop into this feature branch. Also, update the develop-ref version to get past of lots of existing diffs in the output.
- Rerun the full regression test to compare the update version of this feature branch to the newly updated develop-ref.
- That should allow us to see the real diffs to investigate rather than wasting time sifting through lots of other diffs.
After the met-10.1.0-beta5 release, I updated develop-ref to get past diffs from many recent changes. On 1/18/22, I merged the latest from develop into this feature branch, feature_1844_python_embedding, and initiated a regression test on kiowa in: Once the regression test completes, I'll post the list of diffs here. |
I merged the latest develop branch into the feature branch and have the same difference.s I will take a look OBS_THRESH COV_THRESH ALPHA LINE_TYPE TOTAL N_ENS CRPS CRPSS IGN ME RMSE SPREAD ME_OERR RMSE_OERR SPREAD_OER |
The differences at ensemble_stat was fixed and I got the same result. |
Sorry for the delay on this @hsoh-u. I'm going to merge develop into this feature branch and rerun a regression test to confirm. Thanks! |
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.
I approve of these changes.
I ran a full regression test vs develop in kiowa:/d1/projects/MET/MET_pull_requests/met-10.1.0/met-10.1.0_beta6/feature_1844 and confirmed that only diffs are the addition of new output files. I skimmed through the code changes and didn't see any obvious problems. Thanks for adding more tests to unit_python.xml to demonstrate this functionality!
Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu> Co-authored-by: johnhg <johnhg@ucar.edu> Co-authored-by: Seth Linden <linden@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: Howard Soh <hsoh@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@kiowa.rap.ucar.edu> Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com> Co-authored-by: John Halley Gotway <johnhg@seneca.rap.ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: mo-mglover <78152252+mo-mglover@users.noreply.github.com> Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu> Co-authored-by: johnhg <johnhg@ucar.edu> Co-authored-by: Seth Linden <linden@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: Howard Soh <hsoh@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@kiowa.rap.ucar.edu> Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: John Halley Gotway <johnhg@seneca.rap.ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: mo-mglover <78152252+mo-mglover@users.noreply.github.com> Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Co-authored-by: Julie Prestopnik <jpresto@seneca.rap.ucar.edu> Co-authored-by: johnhg <johnhg@ucar.edu> Co-authored-by: Seth Linden <linden@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@ucar.edu> Co-authored-by: j-opatz <59586397+j-opatz@users.noreply.github.com> Co-authored-by: Howard Soh <hsoh@kiowa.rap.ucar.edu> Co-authored-by: John Halley Gotway <johnhg@kiowa.rap.ucar.edu> Co-authored-by: jprestop <jpresto@ucar.edu> Co-authored-by: Seth Linden <linden@ucar.edu> Co-authored-by: hsoh-u <hsoh@ucar.edu> Co-authored-by: John Halley Gotway <johnhg@seneca.rap.ucar.edu> Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu> Co-authored-by: mo-mglover <78152252+mo-mglover@users.noreply.github.com> Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Expected Differences
Added met/src/libcode/vx_pointdata_python
Added two python scripts to met/scripts/python (met_point_obs.py & read_met_point_obs.py: reaing MET point observation NetCDF by python)
Redesign and implement the class structure at met/src/libcode/vx_nc_obs
Added pyobject_as_bool to met/src/libcode/vx_python3_utils
Support python embedding for 4 tools
Added 4 unittests (NetCDF input is changed to python embedding, so the same output should be generated)
Added documentations
Do these changes introduce new tools, command line arguments, or configuration file options? [Yes]
If yes, please describe:
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
Describe testing already performed for these changes:
The outputs are available at /d1/personal/hsoh/git/pull_request/MET_feature_1844_python_embedding/unit_test_output/
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? [No]
If yes, describe the new output and/or changes to the existing output:
Please complete this pull request review by [Fill in date].
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