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

Update Truth: For dtcenter/MET#2981 #2704

Closed
6 of 11 tasks
JohnHalleyGotway opened this issue Sep 26, 2024 · 3 comments
Closed
6 of 11 tasks

Update Truth: For dtcenter/MET#2981 #2704

JohnHalleyGotway opened this issue Sep 26, 2024 · 3 comments
Assignees
Labels
component: CI/CD Continuous integration and deployment issues priority: blocker Blocker requestor: METplus Team METplus Development Team type: update truth Update truth dataset
Milestone

Comments

@JohnHalleyGotway
Copy link
Collaborator

Describe Expected Changes

Updating MTD to use common library code to read/write NetCDF output files results in trivial differences to those MTD NetCDF output files. For example, the order of the lat and lon dimensions are swapped and the amount of precision with which the grid specification is written is modified. While these will be flagged as differences, they can be safely ignored.

Define the Metadata

Title

  • Define the Title of this issue as Update Truth: For dtcenter/{REPO}#{PR_NUMBER} to indicate the repository and pull request that warranted this issue.

Assignee

Assign this issue to the author of the pull request that warranted this issue. Optionally assign anyone else who should review the differences in the output.

  • Select engineer(s) or no engineer required
  • Select scientist(s) or no scientist required

Milestone and Projects

  • Select Milestone as the next official version if updating truth data for the develop branch OR select next METplus-Wrappers-X.Y.Z bugfix version if updating truth data for a main_vX.Y branch.
  • If updating truth data for the develop branch, select the METplus-Wrappers-X.Y.Z Development project OR if updating truth for a main_vX.Y branch, select the Coordinated METplus-X.Y Support project

Update Truth Checklist

  • Review the GitHub Actions workflow that was triggered by the PR merge
    • If no differences were found, note this in a comment.
    • If all of the differences are expected, note this in a comment.
      Include any details of how the review was performed.
    • If unexpected differences are found, the following instructions can
      help uncover potential explanations. If none of these apply and the
      source of the differences cannot be determined, contact the
      METplus wrappers lead engineer (@georgemccabe) for assistance.
      • Search for other open issues that have the label type: update truth
        applied by clicking on the label on this issue. Coordinate with the
        author of these issues to ensure all diffs are properly reviewed.
      • Check if any additional GitHub Actions testing workflows have been
        triggered since the workflow that corresponds to this issue was run.
        Review the latest run to ensure that there are no diffs that are
        unrelated to this issue.
      • If the incorrect differences are caused by the changes from the
        issue that warranted this issue, consider reverting the PR and
        re-opening the issue.
    • Iterate until one of the above conditions apply.
  • Approve the update of the truth data
    • Contact the METplus wrappers lead engineer (@georgemccabe) or
      backup lead (@jprestop) to let them know that the truth data can
      be updated.
  • Update the truth data.
    This should be handled by a METplus wrappers engineer.
    See the instructions to update the truth data
    for more info.
  • Close this issue.
@JohnHalleyGotway JohnHalleyGotway added priority: blocker Blocker alert: NEED ACCOUNT KEY Need to assign an account key to this issue component: CI/CD Continuous integration and deployment issues requestor: METplus Team METplus Development Team type: update truth Update truth dataset labels Sep 26, 2024
@JohnHalleyGotway JohnHalleyGotway added this to the METplus-6.0.0 milestone Sep 26, 2024
@JohnHalleyGotway JohnHalleyGotway self-assigned this Sep 26, 2024
@JohnHalleyGotway
Copy link
Collaborator Author

JohnHalleyGotway commented Sep 27, 2024

@georgemccabe I was expecting that the dtcenter/MET#2981 for issue dtcenter/MET#2979 would trigger diffs in METplus. However, this testing workflow run completed without any differences being flagged.

I do see this NetCDF MTD output file:

met_tool_wrapper/MTD/mtd/2005080706/mtd_WRF_APCP_vs_MC_PCP_APCP_03_A03_20050807_060000V_obj.nc

And see that it's metadata has changed as expected... the order of the dims plus global attribute projection details.

dimensions:
	time = 3 ;
	lat = 129 ;
	lon = 185 ;

However this was not flagged as a difference by the METplus diffing logic. So I chalk this up to a difference in the METplus diffing rules versus what's currently being used in MET.

I'll just close this issue since we do NOT actually need to update the truth data at this time. Just wanted to call your attention to these details. Please feel free to reopen if you'd like to handle this in a different way.

@github-project-automation github-project-automation bot moved this from 🟢 Ready to 🏁 Done in METplus-Analysis-6.0.0 Development Sep 27, 2024
@JohnHalleyGotway JohnHalleyGotway removed the alert: NEED ACCOUNT KEY Need to assign an account key to this issue label Sep 27, 2024
@georgemccabe
Copy link
Collaborator

@JohnHalleyGotway, do you see the differences flagged in MET as something that we should be alerted to in the METplus diff logic? If so, I think this would be a good opportunity to update the METplus logic to include the missing checks.

@JohnHalleyGotway
Copy link
Collaborator Author

I do see that this nc_is_equal(...) function handles the NetCDF diff logic.

I do think we should probably enhance it to diff the global/variable attributes rather than just the data. Obviously, the data is very important, but the metadata is also pretty important. MET runs ncdump -h on each input and then diffs the output, ignoring creation date and path specifics that would be expected to differ. But that adds a dependence on ncdump. So we could also diff entirely in Python.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: CI/CD Continuous integration and deployment issues priority: blocker Blocker requestor: METplus Team METplus Development Team type: update truth Update truth dataset
Projects
No open projects
Status: 🏁 Done
Development

No branches or pull requests

2 participants