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 2302 python embedding converting #2340

Merged
merged 7 commits into from
Nov 18, 2022

Conversation

hsoh-u
Copy link
Collaborator

@hsoh-u hsoh-u commented Nov 10, 2022

Expected Differences

The MET/scripts/python/read_ascii_point.py can be used instead of MET/scripts/python/read_met_point_obs.py for 11 column text input.

  • modified the existing python scripts
  • two data files: no changes for the content. Only file permission is updated (removed the executable permission)
  • unit test:
    • "python_plot_point_obs_CONFIG_XXX" is the same as "plot_point_obs_CONFIG" at unit_plot_point_obs.xml except output directory.
    • Renamed "python_plot_point_obs_CONFIG_XXX" to "python_plot_point_obs_with_point_data" and updated arguments

How to use: add following two lines

from met_point_obs import convert_point_data   # newly added

point_data = pd.read_csv(...).values.tolist()             # existing code by customer
met_point_data = convert_point_data(point_data)   # newly added
  • 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:
  • Added to unit test
  • Converted ascii2nc and pb2nc output at the nightly build output by using pntascci2nc.R and used them as input to plot_point_obs
  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:
  • Converted ascii2nc and pb2nc output at the nightly build output by using pntascci2nc.R and used them as input to plot_point_obs
  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [No]

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

  • Will this PR result in changes to the test suite? [Yes]

    If yes, describe the new output and/or changes to the existing output:

One new plot (precip24_2010010112.ps) is added.

  • Please complete this pull request review by [Fill in date].

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 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.

@JohnHalleyGotway
Copy link
Collaborator

@DanielAdriaansen this one is the same situation. We're both assigned, but I'd really like to get your input on these changes to keep you in the loop on python embedding functionality in MET.

In particular I don't see any changes to the User's Guide on this PR... and it sure seems like we'd need some to document the changes to the functionality.

@hsoh-u hsoh-u changed the title Feature 2285 python embedding converting Feature 2302 python embedding converting Nov 16, 2022
@hsoh-u
Copy link
Collaborator Author

hsoh-u commented Nov 16, 2022

The associated issue corrected to this PR

@DanielAdriaansen
Copy link
Contributor

This work adds a converter to transform a point_data object to met_point_data, if the user has point_data but wants to use a MET tool that requires met_point_data.

read_ascii_point.py has been modified such that both point_data and met_point_data exist when it is called, thus having both objects available for whatever tool is being used downstream of the Python embedding, if read_ascii_point.py is being used.

If a user has existing Python embedding with point_data but wants to use a tool that requires met_point_data, they will have to add the following lines to their Python embedding script:

from met_point_obs import convert_point_data
met_point_data = convert_point_data(point_data)

After #2285 is complete, the user won't have to do this. They can create point_data (which is far easier to create), and something within MET will sense that they have point_data and call convert_point_data for the user if the tool downstream of the Python embedding they are using requires met_point_data.

I will add some documentation updates soon, and then add my review.

@DanielAdriaansen
Copy link
Contributor

For testing, I did the following:

  1. ascii2nc with the proposed read_ascii_point.py using /d1/projects/MET/MET_releases/MET/data/sample_obs/ascii/sample_ascii_obs.txt on seneca, to create a "point NetCDF file":
/d1/projects/MET/MET_regression/develop/NB20221118/MET-develop/bin/ascii2nc -format python "scripts/python/read_ascii_point.py /d1/projects/MET/MET_releases/MET/data/sample_obs/ascii/sample_ascii_obs.txt" ascii2nc_python.nc
  1. plot_point_obs with output from (1):
/d1/projects/MET/MET_regression/develop/NB20221118/MET-develop/bin/plot_point_obs ascii2nc_python.nc ascii2nc_python.ps
  1. plot_point_obs with proposed read_ascii_point.py using Python embedding and /d1/projects/MET/MET_releases/MET/data/sample_obs/ascii/sample_ascii_obs.txt on seneca:
/d1/projects/MET/MET_regression/develop/NB20221118/MET-develop/bin/plot_point_obs "PYTHON_NUMPY=scripts/python/read_ascii_point.py /d1/projects/MET/MET_releases/MET/data/sample_obs/ascii/sample_ascii_obs.txt" new.ps

Since Test 3 uses plot_point_obs which requires the "MET NetCDF point observation format" as input -or- the met_point_data object via Python embedding, it should succeed now with Howard's changes here, when previously it would fail.

With Howard's changes, Test 3 succeeds now.

I am adding some documentation updates and then I will add my review.

…tions in Appendix F, and adds a link for the section on Python embedding for MPR data.
…s that is better suited for Appendix F. Appendix F is already referenced in describing ASCII2NC. Cleaned up some wording regarding Point2Grid Python embedding.
@DanielAdriaansen
Copy link
Contributor

NOTE:

I removed a section from the "Reformatting of Point Observations" that described Python Embedding with ascii2nc. I felt that it was more appropriate to have this in Appendix F:
af71f3b

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 approve.

@hsoh-u hsoh-u merged commit a96575c into develop Nov 18, 2022
JohnHalleyGotway added a commit that referenced this pull request Nov 19, 2022
Co-authored-by: MET Tools Test Account <[email protected]>
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: Howard Soh <[email protected]>
Co-authored-by: Dave Albo <[email protected]>
Co-authored-by: John Halley Gotway <[email protected]>
Co-authored-by: johnhg <[email protected]>
Co-authored-by: Lisa Goodrich <[email protected]>
Co-authored-by: jprestop <[email protected]>
Co-authored-by: j-opatz <[email protected]>
Co-authored-by: George McCabe <[email protected]>
Co-authored-by: Julie Prestopnik <[email protected]>
Co-authored-by: Jonathan Vigh <[email protected]>
Co-authored-by: Seth Linden <[email protected]>
Co-authored-by: hsoh-u <[email protected]>
Co-authored-by: bikegeek <[email protected]>
Co-authored-by: davidalbo <[email protected]>
Co-authored-by: lisagoodrich <[email protected]>
Co-authored-by: Daniel Adriaansen <[email protected]>
@JohnHalleyGotway JohnHalleyGotway deleted the feature_2285_python_embedding_converting branch February 28, 2023 19:59
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
3 participants