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 #2339 stat_analysis_seeps #2343

Merged

Conversation

JohnHalleyGotway
Copy link
Collaborator

@JohnHalleyGotway JohnHalleyGotway commented Nov 12, 2022

@hsoh-u I'd like to merge my Stat-Analysis changes for SEEPS back into your feature branch.
** IMPORTANT ** there's memory management problems in computing the seeps density that are fixed here using STL vectors.

These Stat-Analysis change include...

  • Supporting -job filter for -line_type SEEPS and -line_type SEEPS_MPR.
  • Supporting -job summary for columns in SEEPS and/or SEEPS_MPR.
  • Supporting -job aggregate for -line_type SEEPS.
  • Supporting -job aggregate_stat for -line_type SEEPS_MPR -out_line_type SEEPS.
  • Add tests to test_unit/xml/unit_stat_analysis_ps.xml to demonstrate.
  • Update User's Guide Stat-Analysis chapter to list the supported SEEPS functions.

Expected Differences

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

    The Stat-Analysis jobs that I added demonstrate that the filtering, summary, and aggregation logic.
    But that logic is VERY simplistic.

    • Aggregates all SEEPS columns (S12 S13 S21 S23 S31 S32 PF1 PF2 PF3 PV1 PV2 PV3 MEAN_FCST MEAN_OBS SEEPS) as weighted averages where the weight is defined by the TOTAL column.
    • To aggregate SEEPS_MPR to SEEPS, it loads the SEEPS_MPR data into a PairDataPoint object and calls the library code toe compute SEEPS.
  • Recommend testing for the reviewer(s) to perform, including the location of input datasets, and any additional instructions:

    • Biggest question is the aggregation logic. I think this simple approach is fine for most columns. But I have NO IDEA about the S, PF, and PV columns. Those are written as 0 in the output of Stat-Analysis. Once you have their contents figured out, we should review/revise the aggregation logic as needed.
  • Do these changes include sufficient documentation updates, ensuring that no errors or warnings exist in the build of the documentation? [Yes]

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

    Adds new Stat-Analysis output.

  • Please complete this pull request review by [Mon 11/14/22].

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 JohnHalleyGotway added this to the MET 11.0.0 milestone Nov 12, 2022
@JohnHalleyGotway JohnHalleyGotway linked an issue Nov 12, 2022 that may be closed by this pull request
22 tasks
@JohnHalleyGotway
Copy link
Collaborator Author

@hsoh-u, FYI, I saw an error in the docker build step as well as a documentation warning message. I've fixed both in my feature branch and will let GHA finish running to confirm that the fixes work as expected.

@JohnHalleyGotway JohnHalleyGotway merged commit 335b973 into feature_1943_gridstat_seeps Nov 12, 2022
@JohnHalleyGotway JohnHalleyGotway deleted the feature_2339_stat_analysis_seeps branch November 12, 2022 19:04
@hsoh-u hsoh-u mentioned this pull request Nov 14, 2022
15 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.

Enhance Stat-Analysis to aggregate SEEPS_MPR and SEEPS line types.
2 participants