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.

Sign up for GitHub

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

Jump to bottom

Bugfix #2575 main_v11.0 python_convert #2577

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Bugfix #2575 main_v11.0 python_convert #2577

merged 1 commit into from
Jun 15, 2023

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Jun 14, 2023 •
edited
Loading

Expected Differences

This PR fixes the #2575. Please see PR #2576 which has details about fixing it in the develop branch and review them at the same time.

  • 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

Please see #2576

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.

@JohnHalleyGotway
Per #2575, call process_data_plane() on a copy of the DataPlane store…
c370145 
…d in the MetPythonDataFile class rather than the original one. Otherwise, retrieving the data multiple times will cause the same conversion operation to be performed multiple times.
@JohnHalleyGotway JohnHalleyGotway added this to the MET 11.0.3 (bugfix) milestone Jun 14, 2023
@JohnHalleyGotway JohnHalleyGotway linked an issue Jun 14, 2023 that may be closed by this pull request
Bugfix: Fix logic for Python embedding with data censoring and/or conversion #2575
Closed
23 tasks
@JohnHalleyGotway JohnHalleyGotway requested a review from CPKalb June 15, 2023 14:42
@JohnHalleyGotway
Copy link
Collaborator Author

JohnHalleyGotway commented Jun 15, 2023 •
edited
Loading

Adding @CPKalb as a reviewer as well. Please find the code for this PR compiled on seneca in /d1/projects/MET/MET_pull_requests/met-11.0.3/issue_2575/MET-bugfix_2575_main_v11.0_python_convert_into_main_v11.0/bin.

@CPKalb please let me know if you're able to confirm that this change fixes the "double" application of your convert function.

DanielAdriaansen
DanielAdriaansen approved these changes Jun 15, 2023
View reviewed changes
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.

Same review as for #2575:

The code change looks straightforward. My only concern was whether this may be a wider problem, but per the docs here:
https://met.readthedocs.io/en/latest/Users_Guide/config_options.html?highlight=convert(x)#regrid

it looks like convert(x) only applies to dataplanes and not point data or other tools so I think this should squash the bug for all cases.

I also tested (using @JohnHalleyGotway run.sh script on seneca) with MET_PYTHON_EXE set and it appears the change works for that case (and the bug was also present for that case in the old version).

I approve.

CPKalb
CPKalb approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@CPKalb CPKalb left a comment

Choose a reason for hiding this comment

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

Tested and verified that my data is only converted once.

@JohnHalleyGotway JohnHalleyGotway merged commit 9fc8ea9 into main_v11.0 Jun 15, 2023
@JohnHalleyGotway JohnHalleyGotway deleted the bugfix_2575_main_v11.0_python_convert branch June 15, 2023 17:17
JohnHalleyGotway pushed a commit that referenced this pull request Sep 11, 2023
@github-actions
Update main_v11.0-ref after 11.0.3 release (#2678) (#2679)
42f672e 
Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu>
Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: hsoh-u <hsoh@ucar.edu>
Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com>
Co-authored-by: Stephen Herbener <32968781+srherbener@users.noreply.github.com>
fix #2389 main_v11.0 flowchart (#2391)
fix definitions of G172 and G220 based on comments in NOAA-EMC/NCEPLIBS-w3emc#157. (#2405)
fix #2380 main_v11.0 override (#2381)
fix #2408 main_v11.0 empty config (#2409)
fix #2390 main_v11.0 fix compiling hdf5 with zlib and handle NetCDF-C zip (#2403)
fix #2415 main_v11.0 modulefiles (#2416)
fix #2412 main_v11.0 climo (#2420)
fix #2426 main_v11.0 buoy (#2432)
fix #2437 main_v11.0 convert (#2438)
fix for main_v11.0, for #2437, forgot one reference to the search_parent for a dictionary lookup.
fix 2428 python from env main v11.0 (#2443)
fix 2428 python csv input (#2450)
fix #2452 main_v11.0 airnow (#2453)
fix #2402 main_v11.0 sonarqube (First PR) (#2447)
fix #2449 main_v11.0 pdf (#2465)
fix 2428 python csv input (#2467)
fix #2514 main_v11.0 clang (#2556)
fix #2575 main_v11.0 python_convert (#2577)
fix 2596 main v11.0 rpath compilation (#2600)
fix 2596 main v11.0 rpath compilation (#2613)
fix #2644 main_v11.0 percentile (#2645)
JohnHalleyGotway added a commit that referenced this pull request Dec 18, 2023
@JohnHalleyGotway
Update main_v11.0-ref after #2761 upgraded seneca to Debian Bookworm (#…
2bf872c 
…2770)

Co-authored-by: MET Tools Test Account <met_test@seneca.rap.ucar.edu>
Co-authored-by: Howard Soh <hsoh@seneca.rap.ucar.edu>
Co-authored-by: John Halley Gotway <johnhg@ucar.edu>
Co-authored-by: jprestop <jpresto@ucar.edu>
Co-authored-by: hsoh-u <hsoh@ucar.edu>
Co-authored-by: George McCabe <23407799+georgemccabe@users.noreply.github.com>
Co-authored-by: lisagoodrich <33230218+lisagoodrich@users.noreply.github.com>
Co-authored-by: Stephen Herbener <32968781+srherbener@users.noreply.github.com>
fix #2389 main_v11.0 flowchart (#2391)
fix definitions of G172 and G220 based on comments in NOAA-EMC/NCEPLIBS-w3emc#157. (#2405)
fix #2380 main_v11.0 override (#2381)
fix #2408 main_v11.0 empty config (#2409)
fix #2390 main_v11.0 fix compiling hdf5 with zlib and handle NetCDF-C zip (#2403)
fix #2415 main_v11.0 modulefiles (#2416)
fix #2412 main_v11.0 climo (#2420)
fix #2426 main_v11.0 buoy (#2432)
fix #2437 main_v11.0 convert (#2438)
fix for main_v11.0, for #2437, forgot one reference to the search_parent for a dictionary lookup.
fix 2428 python from env main v11.0 (#2443)
fix 2428 python csv input (#2450)
fix #2452 main_v11.0 airnow (#2453)
fix #2402 main_v11.0 sonarqube (First PR) (#2447)
fix #2449 main_v11.0 pdf (#2465)
fix 2428 python csv input (#2467)
fix #2514 main_v11.0 clang (#2556)
fix #2575 main_v11.0 python_convert (#2577)
fix 2596 main v11.0 rpath compilation (#2600)
fix 2596 main v11.0 rpath compilation (#2613)
fix #2644 main_v11.0 percentile (#2645)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CPKalb CPKalb CPKalb approved these changes

@DanielAdriaansen DanielAdriaansen DanielAdriaansen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Coordinated METplus-5.0 Support
Status: ✅ Done
Milestone
MET 11.0.3 (bugfix)
Development

Successfully merging this pull request may close these issues.

Bugfix: Fix logic for Python embedding with data censoring and/or conversion
3 participants
@JohnHalleyGotway @DanielAdriaansen @CPKalb