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

Bugfix: Fix GridStat_SeriesAnalysis _fcstNMME_obsCPC _seasonal_forecast use cases with poorly configured climatology settings #2695

Closed
8 of 24 tasks
JohnHalleyGotway opened this issue Sep 18, 2024 · 6 comments
Labels
alert: NEED ACCOUNT KEY Need to assign an account key to this issue METplus: Configuration priority: high High Priority requestor: METplus Team METplus Development Team type: bug Fix something that is not working
Milestone

Comments

@JohnHalleyGotway
Copy link
Collaborator

JohnHalleyGotway commented Sep 18, 2024

Describe the Problem

Merging pull request dtcenter/MET#2963 for the forecast climo config options of issue dtcenter/MET#2924 triggered this METplus Testing Workflow run. Two of the 56 use case groups had errors related to climatology data. Upon inspection, we found that the GridStat_SeriesAnalysis _fcstNMME_obsCPC _seasonal_forecast use case fails to supply the climo data as expected.

Expected Behavior

In Use Case Tests (s2s:0) the GridStat_SeriesAnalysis _fcstNMME_obsCPC _seasonal_forecast use case fails with the following error message:

egrep -i error series_analysis.full_stats.log | sort -u
ERROR  : Dictionary::lookup_string() -> lookup failed for name "level"

The issue is that the climo_mean file name is provided (on this line) but no field list is provided to specify what climo data should be read from that file. Fixing this may be as simple as adding the SERIES_ANALYSIS_CLIMO_MEAN_USE_FCST config option. However, you'd need to confirm that the forecast and climo data field lists really should use the same name and level settings.

Note that I did confirm that the existing use case IS NOT providing climo data as expected. I pulled the existing METplus truth data, ran a command to extract the Series-Analysis NetCDF output file, and used ncview to display it:

docker create --name output-s2s_0 dtcenter/metplus-data-dev:output-develop-s2s_0
docker run -it --rm --volumes-from output-s2s_0 -v `pwd`/window:/window  dtcenter/met:12.0.0-beta5 cp /data/truth/s2s/GridStat_SeriesAnalysis_fcstNMME_obsCPC_seasonal_forecast/model_applications/s2s/GridStat_SeriesAnalysis_fcstNMME_obsCPC_seasonal_forecast/SeriesAnalysis/series_analysis_NMME_CPC_stats_F2m_full_stats.nc /window/.
ncview window/series_analysis_NMME_CPC_stats_F2m_full_stats.nc

And the field of series_cnt_ANOM_CORR contains all bad data. That means that no climo mean data was actually passed to series_analysis.

Relevant Deadlines

List relevant project deadlines here or state NONE.

Funding Source

Define the source of funding and account keys here or state NONE.

Define the Metadata

Assignee

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

Labels

  • Review default alert labels
  • Select component(s)
  • Select priority
  • Select requestor(s)

Milestone and Projects

  • Select Milestone as the next bugfix version
  • Select Coordinated METplus-X.Y Support project for support of the current coordinated release
  • Select METplus-Wrappers-X.Y.Z Development project for development toward the next official release

Define Related Issue(s)

Consider the impact to the other METplus components.

Bugfix Checklist

See the METplus Workflow for details.

  • Complete the issue definition above, including the Time Estimate and Funding Source.

  • Fork this repository or create a branch of main_<Version>.
    Branch name: bugfix_<Issue Number>_main_<Version>_<Description>

  • Fix the bug and test your changes.

  • Add/update log messages for easier debugging.

  • Add/update unit tests.

  • Add/update documentation.

  • Add any new Python packages to the METplus Components Python Requirements table.

  • Push local changes to GitHub.

  • Submit a pull request to merge into main_<Version>.
    Pull request: bugfix <Issue Number> main_<Version> <Description>

  • Define the pull request metadata, as permissions allow.
    Select: Reviewer(s) and Development issue
    Select: Milestone as the next bugfix version
    Select: Coordinated METplus-X.Y Support project for support of the current coordinated release

  • Iterate until the reviewer(s) accept and merge your changes.

  • Delete your fork or branch.

  • Complete the steps above to fix the bug on the develop branch.
    Branch name: bugfix_<Issue Number>_develop_<Description>
    Pull request: bugfix <Issue Number> develop <Description>
    Select: Reviewer(s) and Development issue
    Select: Milestone as the next official version
    Select: METplus-Wrappers-X.Y.Z Development project for development toward the next official release

  • Close this issue.

@JohnHalleyGotway JohnHalleyGotway added type: bug Fix something that is not working priority: high High Priority alert: NEED ACCOUNT KEY Need to assign an account key to this issue requestor: METplus Team METplus Development Team METplus: Configuration labels Sep 18, 2024
@JohnHalleyGotway JohnHalleyGotway added this to the METplus-6.0.0 milestone Sep 18, 2024
@JohnHalleyGotway JohnHalleyGotway changed the title Bugfix: Fix two existing METplus use cases with poorly configured climatology settings Bugfix: Fix GridStat_SeriesAnalysis _fcstNMME_obsCPC _seasonal_forecast use cases with poorly configured climatology settings Sep 18, 2024
@j-opatz
Copy link
Contributor

j-opatz commented Sep 26, 2024

I've tweaked a few things to the existing configuration file, including adding climo field information and updating the configuration file arguments requesting line types. However, the use case still seems to be unable to process the incoming climatology field. The ANOM_CORR and RMSFA variable fields are empty.

Additional tests of bogus data as climo data also ended in no climatology being read in.

Next step will be to build a simplified version of this configuration file, only allowing 1 run (rather than the 6 months of lead time) and see if something comes across in the ANOM_CORR and RMFSA fields. If it continues to prove elusive or consumes too much analysis time this use case may be removed from the METplus repository, with the primary reason being if the climatology function does not work, then the purpose of the use case is null.

@j-opatz
Copy link
Contributor

j-opatz commented Sep 30, 2024

Continued testing today. It seems that there may be a bigger issue at hand: when I did a simple GridStat run with nothing additional other than fcst, obs, and climo mean information, the nc_flag option does not put out a climo field entry. There's nothing in the log saying the read in was unsuccessful, though. It seems like a recent change may have stopped METplus from creating climo-related output.

As a second check, I will run a direct call to Grid-Stat via MET using the exact same set up (files, fields, etc) and see if nc_flags will produce a climo field. If not, additional eyes may be needed on this issue.

@j-opatz
Copy link
Contributor

j-opatz commented Oct 2, 2024

Issue was discovered after running a Grid-Stat test on the same data as a single run of SeriesAnalysis. The output from the GridStat run outputs the data used as climatology in the second SeriesAnalysis call; however, that data has a valid time stamp of 19700101. During a check of day_interval it's well beyond the 31 default days, so it's skipped. By adding

SERIES_ANALYSIS_CLIMO_MEAN_DAY_INTERVAL = NA

to the run, the output with climatology is generated.

I'll create a new bugfix branch that captures the necessary adjustments to the config file and get a PR set up later today.

@georgemccabe
Copy link
Collaborator

@j-opatz, you could add this fix to my branch feature_2710_use_case_cleanup to include it with the changes for #2710, so we can merge these changes update the truth data once.

@j-opatz
Copy link
Contributor

j-opatz commented Oct 2, 2024

Just merged branch 2695 into branch 2710, so branch 2710 should now contain the fixes. Thanks @georgemccabe!

@georgemccabe
Copy link
Collaborator

Fixed in PR #2711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
alert: NEED ACCOUNT KEY Need to assign an account key to this issue METplus: Configuration priority: high High Priority requestor: METplus Team METplus Development Team type: bug Fix something that is not working
Projects
Status: 🏁 Done
Development

No branches or pull requests

3 participants