Skip to content

Conversation

@JorgeBornemann
Copy link
Collaborator

@JorgeBornemann JorgeBornemann commented Jan 27, 2025

Fixes #653

Contribution checklist

Aim to have all relevant checks ticked off before merging. See the developer's guide for more detail.

  • Documentation has been updated to reflect change.
  • New code has tests, and affected old tests have been updated.
  • All tests and CI checks pass.
  • Ensured the pull request title is descriptive.
  • Conda lock files have been updated if dependencies have changed.
  • Attributed any Generative AI, such as GitHub Copilot, used in this PR.
  • Marked the PR as ready to review.

@JorgeBornemann JorgeBornemann linked an issue Jan 27, 2025 that may be closed by this pull request
@JorgeBornemann JorgeBornemann marked this pull request as draft January 27, 2025 09:10
@github-actions
Copy link
Contributor

Coverage

@JorgeBornemann
Copy link
Collaborator Author

Initial implementation of NG-VER METPlus PointStat configurations for the Met Office.

@jfrost-mo jfrost-mo marked this pull request as ready for review January 27, 2025 09:17
@jfrost-mo jfrost-mo marked this pull request as draft January 27, 2025 09:19
@jfrost-mo
Copy link
Member

Thanks Jorge, its great to see this!

I'll probably have a chance to review towards the end of the week, so let me know if there is anything to pay particular attention to.

@JorgeBornemann
Copy link
Collaborator Author

At the moment is just sanity check of the workflow structure and the apps/wrappers. If you think it is sound, it will need someone from the Met Office who has access to the observations and forecasts files to take it over, finish the coding and test thoroughly. Any change beyond the Met Office specific files, I need to be aware of it, as it will impact other sites.

@JorgeBornemann
Copy link
Collaborator Author

One thing to pay special attention to is whether there is in this branch information that should go in the restricted repo.

@jfrost-mo jfrost-mo changed the title 653 add pointstat met office configuration Add MET pointstat configuration for the Met Office Jul 24, 2025
Copy link
Member

@jfrost-mo jfrost-mo left a comment

Choose a reason for hiding this comment

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

Per our discussion today, I've only reviewed the non .conf files, as the .conf files were copied from elsewhere.

Similarly, we should check with Rachel whether there is a more up-to-date branch that we should be using instead.

But otherwise this change looks sensible, and aside from a few things noted in the comments below, it should be fine to merge eventually.

[[[environment]]]
{% if METPLUS_GRID_STAT|default(False) %}
METPLUS_ANA_DIR = {{METPLUS_ANA_DIR}}
{% endif %}
Copy link
Member

Choose a reason for hiding this comment

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

Stray endif?

# Runs for every forecast initiation time to process the data in parallel.
{% for date in CSET_CASE_DATES %}
R1/{{date}} = """
metplus_grid_stat => housekeeping_full
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
metplus_grid_stat => housekeeping_full
metplus_grid_stat => housekeeping

The task is now just housekeeping.

{% elif CSET_CYCLING_MODE == "trial" %}
# Analyse from each forecast.
{{CSET_TRIAL_CYCLE_PERIOD}} = """
metplus_grid_stat => housekeeping_full
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
metplus_grid_stat => housekeeping_full
metplus_grid_stat => housekeeping

metplus_ascii2nc_{{metplus_obs_type}}
{% endfor %}
{% for metplus_stat in METPLUS_POINTSTAT_STATS %}
[METPLUS_ASCII2NC]:suceed-all => metplus_point_stat_{{metplus_stat}} => housekeeping_full
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[METPLUS_ASCII2NC]:suceed-all => metplus_point_stat_{{metplus_stat}} => housekeeping_full
[METPLUS_ASCII2NC]:suceed-all => metplus_point_stat_{{metplus_stat}} => housekeeping

metplus_ascii2nc_{{metplus_obs_type}}
{% endfor %}
{% for metplus_stat in METPLUS_POINTSTAT_STATS %}
[METPLUS_ASCII2NC]:suceed-all => metplus_point_stat_{{metplus_stat}} => housekeeping_full
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
[METPLUS_ASCII2NC]:suceed-all => metplus_point_stat_{{metplus_stat}} => housekeeping_full
[METPLUS_ASCII2NC]:suceed-all => metplus_point_stat_{{metplus_stat}} => housekeeping

Copy link
Member

Choose a reason for hiding this comment

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

These tasks should all depend on setup_complete[^], which ensures all the cold_start stuff is complete (environment creation, etc.)

They should probably also depend on fetch_complete which is a per-cycle marker indicating that all the needed data has been retrieved.

Copy link
Member

Choose a reason for hiding this comment

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

These tasks should all depend on setup_complete[^], which ensures all the cold_start stuff is complete (environment creation, etc.)

They should probably also depend on fetch_complete which is a per-cycle marker indicating that all the needed data has been retrieved.


#no set -xeu as check errors in next task

#no set -xeu as check errors in next task
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#no set -xeu as check errors in next task

Remove duplicate line.

#no set -xeu as check errors in next task

#no set -xeu as check errors in next task
export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:/data/users/cfrd/MET_requirements/MET9/lib/:/usr/lib64/:/net/project/ukmo/scitools/opt_scitools/conda/deployments/default-current/lib
Copy link
Member

Choose a reason for hiding this comment

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

While this is the Met Office specific file, could we abstract these paths out into the configuration? Especially as these paths are out of date.

Copy link
Member

Choose a reason for hiding this comment

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

CYLC_SUITE_DEF_PATH is a cylc 7ism. It should be replaced with CYLC_WORKFLOW_RUN_DIR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add PointStat Met Office configuration

3 participants