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.

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 #2285 read_point_data #2509

Merged
merged 88 commits into from
Apr 20, 2023
Merged

Feature #2285 read_point_data #2509

merged 88 commits into from
Apr 20, 2023

Conversation

hsoh-u
Copy link
Collaborator

@hsoh-u hsoh-u commented Apr 10, 2023

Expected Differences

The python object point_data (11 column data) is supported MET as is. The previous way was converting "point_data" to "met_point_data" from met_point_obs.py by calling "convert_point_data".

The unit test for converting "point_data" to "met_point_data" by python script is changed to not call "convert_point_data".

Some common API with ascci2nc (filtering) is moved to src/libcode/vx_pointdata_python

The python scripts are re-structured and renamed Python Embedding Organizartion:

  • The python scripts at data/wrappers are moved to scripts/python/pyembed

  • The python scripts at scripts/utility are moved to scripts/python/utility

  • The python scripts at scripts/python are moved to scripts/python/met and scripts/python/examples

  • scripts/python/met_point_obs.py is renamed to scripts/python/met/point.py

  • scripts/python/met_point_obs_nc.py is merged to scripts/python/met/point.py

  • scripts/python/met/dataplane.py is added (moved from dataplane related APIs, numpy/xarray)

  • scripts/python/met/mprbase.py is added (moved from MPR related APIs)

  • scripts/python/pyembed/python_embedding.py is added (moved common APIs for python embedding)

  • Do these changes introduce new tools, command line arguments, or configuration file options? [No]

    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:

Unittest and the same outputs

  • 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]

The path for the python scripts were updated for the user guide.
I did not check the materials for tutorials.

  • Do these changes include sufficient testing updates? [Yes]

The existing unittest passes the python object "point_data" instead of "met_python_data" object which is converted from "point_data" object.

  • 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:

  • Please complete this pull request review by [4/12/2023].

Pull Request Checklist

See the METplus Workflow for details.

  • Review the source issue metadata (required labels, projects, and milestone).
  • 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)
    Select: Organization level software support Project or Repository level development cycle Project
    Select: Milestone as the version that will include these changes
  • After submitting the PR, select Development issue 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.

Howard Soh added 29 commits March 9, 2023 16:46
Copy link
Contributor

@DanielAdriaansen DanielAdriaansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tested these changes on Seneca. I performed the following tests:

  1. plot_point_obs with Python embedding with convert=True inside read_ascii_point.py
  2. plot_point_obs with Python embedding with convert=False inside read_ascii_point.py
  3. Tests 1&2, but with MET_PYTHON_EXE set to force reading/writing temporary files
  4. plot_data_plane with compile time Python
  5. plot_data_plane with MET_PYTHON_EXE set to force reading/writing temporary files
  6. stat_analysis for the MPR Python Embedding

All of the above tests were successful on Seneca in my testing area.

I did not do an extensive review of the C++, @JohnHalleyGotway may be a better reviewer for those changes than myself.

I also included a complete overhaul of Appendix F, and linked two additional issues that this PR will close with those changes.

I approve of these changes. Thanks @hsoh-u!

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.

@DanielAdriaansen, I reviewed the documentation and added a few suggested changes. Please let me know if you have any questions. A huge thank you to you for all of your time and effort that you put into improving this documentation for users (and for our team). I have a feeling it will be very useful. Also, thank you for your attention to detail in staying consistent formatting with the existing documentation. It is much appreciated!

DanielAdriaansen and others added 2 commits April 17, 2023 12:10
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: jprestop <jpresto@ucar.edu>
@DanielAdriaansen
Copy link
Contributor

@DanielAdriaansen, I reviewed the documentation and added a few suggested changes. Please let me know if you have any questions. A huge thank you to you for all of your time and effort that you put into improving this documentation for users (and for our team). I have a feeling it will be very useful. Also, thank you for your attention to detail in staying consistent formatting with the existing documentation. It is much appreciated!

Great, thank YOU for taking the time to read through it and make suggestions. My hope is that this far surpasses the previous Appendix F and will be useful for all, so glad to hear you think so. Appreciate it @jprestop !

Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hsoh-u and @DanielAdriaansen thanks for all your work on getting these details straight.

Howard, let's proceed with the merge now.

@hsoh-u hsoh-u merged commit 0071adf into develop Apr 20, 2023
@hsoh-u hsoh-u deleted the feature_2285_read_point_data branch April 20, 2023 20:01
@JohnHalleyGotway JohnHalleyGotway changed the title Feature 2285 read point data Feature #2285 read_point_data Apr 23, 2023
georgemccabe added a commit to dtcenter/METplus that referenced this pull request Apr 27, 2023
…removes the need for this call -- MET handles it directly, ci-skip-unit-tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
4 participants