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 642 distance map tb #825

Merged
merged 28 commits into from Mar 11, 2021
Merged

Conversation

CPKalb
Copy link
Contributor

@CPKalb CPKalb commented Mar 1, 2021

Pull Request Testing

  • Describe testing already performed for these changes:

    The cases were run and the output was verified to be identical to the original script

  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

    Run the case to be sure it works and compare netcdf distance maps output to original

  • Do these changes include sufficient documentation and testing updates? [Yes or No]
    Yes

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

    Yes, there will be new output data for the new cases

Pull Request Checklist

See the METplus Workflow for details.

  • 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), Project(s), and Milestone
  • After submitting the PR, select Linked Issues 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.

@CPKalb CPKalb requested a review from georgemccabe March 1, 2021 19:01
Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

There are a few things that should be updated. See the notes on the files.

@georgemccabe
Copy link
Collaborator

@CPKalb could you please fill out the template for reviewing this pull request?

@CPKalb CPKalb added this to the METplus-4.0.0 milestone Mar 2, 2021
@CPKalb CPKalb added this to In progress in METplus-4.0.0-rc1 (5/4/21) via automation Mar 2, 2021
@CPKalb
Copy link
Contributor Author

CPKalb commented Mar 2, 2021

What is the test suite?

@georgemccabe
Copy link
Collaborator

What is the test suite?

The automated tests run in GitHub Actions. This PR will change the output because there will be new output data from the new cases.

@CPKalb CPKalb linked an issue Mar 2, 2021 that may be closed by this pull request
21 tasks
@georgemccabe
Copy link
Collaborator

An update on this review: I looked over the changes and they look good. Both cases ran properly on kiowa. The use cases have their own MET config file, which we are moving away from in favor of using GRID_STAT_MET_CONFIG_OVERRIDES and MODE_MET_CONFIG_OVERRIDES. I made some changes to do this, however there are changed in #768 that will be needed to switch to the new format for MODE because grid_res is set in this case and that needs to be set via a METplus config. Once the changes for #768 are merged into develop, I can test that the MODE case runs as expected and approve this PR.

@georgemccabe georgemccabe self-requested a review March 8, 2021 21:51
@CPKalb
Copy link
Contributor Author

CPKalb commented Mar 8, 2021

Okay. I believe that grid_res was set, as well as some of the mode specific parameters in the function to identify objects. Let me know if there is anything I can do to help.

METplus-4.0.0-rc1 (5/4/21) automation moved this from In progress to Pull Request Review Mar 11, 2021
Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

I made the updates to the config files but 1 of the cases failed due to a typo. It is re-running now. If all of the jobs succeed in the push run for this branch, then I will approve this PR. The pull request run will have differences because there is new data and the names of one of the use case groups changed (from 6+ to 6).

Push run:
https://github.com/dtcenter/METplus/actions/runs/641300331

Pull request run:
https://github.com/dtcenter/METplus/actions/runs/641300429

@georgemccabe georgemccabe self-requested a review March 11, 2021 00:44
@georgemccabe
Copy link
Collaborator

Update: I confirmed that the new use cases run successfully. I am updating the logic for running the use case tests so that it will be easier to see that the only change to the test output is the addition of new data from this use case. I will apply those changes to this branch and approve.

Copy link
Collaborator

@georgemccabe georgemccabe left a comment

Choose a reason for hiding this comment

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

Approving this PR and creating a new one from branch feature_642_test_ci_updates for the changes to GitHub Actions

@georgemccabe georgemccabe merged commit da74ebf into develop Mar 11, 2021
METplus-4.0.0-rc1 (5/4/21) automation moved this from Pull Request Review to Done Mar 11, 2021
@georgemccabe georgemccabe deleted the feature_642_distance_map_tb branch March 11, 2021 21:24
@georgemccabe georgemccabe mentioned this pull request Mar 12, 2021
10 tasks
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
Development

Successfully merging this pull request may close these issues.

Add Distance Map use-case for Brightness Temperature
2 participants