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

Synchronize xppm and yppm #39

Merged
merged 2 commits into from
Jan 29, 2025

Conversation

romanc
Copy link
Contributor

@romanc romanc commented Jan 28, 2025

Description

Synchronize xppm.py and yppm.py. In particular, propagate an orchestration call from yppm.py to xppm.py.

There is script to generate yppm from xppm. That script was having outdated paths. Fixed that and and added other fixes such that it generates the expected output again. Also changed gsed to sed assuming people (or CI who I guess is eventually supposed to run the script) are running on non-BSD linux versions, which comes by default with the gnu flavor of sed.

How Has This Been Tested?

  • Ran xppm and yppm tests locally
  • Ran covert_xppm_yppm.sh locally which generated no changes to yppm.py.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas: N/A
  • I have made corresponding changes to the documentation: N/A
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules: N/A
  • New check tests, if applicable, are included: N/A
  • Targeted model if this changed was triggered by a model need/shortcoming: N/A

Copy the orchestration call from yppm back to xppm to synchronize the
two.
Path to stencils changed when importing files into this repo. Adapted
that path.

I also changed `gsed` to `sed` because on (nonBSD) linux systems, the
default `sed` flavor is gnu `sed`.
@romanc romanc force-pushed the romanc/sync-xppm-yppm branch from 10b067d to f215dc1 Compare January 28, 2025 16:04
@romanc romanc mentioned this pull request Jan 28, 2025
8 tasks
@fmalatino fmalatino requested review from FlorianDeconinck and fmalatino and removed request for fmalatino January 28, 2025 17:13
Copy link
Collaborator

@FlorianDeconinck FlorianDeconinck left a comment

Choose a reason for hiding this comment

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

LGTM

@romanc
Copy link
Contributor Author

romanc commented Jan 29, 2025

Just an idea (for a follow-up PR): I think it would be easy/trivial to create a github workflow that uses convert_xppm_yppm.sh to ensure xppm.py and yppm.py are in sync. We could even fit it in the linting one.

@FlorianDeconinck
Copy link
Collaborator

We definitely could. It's not really a priority and the PPM code will not change

@FlorianDeconinck FlorianDeconinck merged commit 3531fc6 into NOAA-GFDL:develop Jan 29, 2025
4 checks passed
@romanc romanc deleted the romanc/sync-xppm-yppm branch January 30, 2025 08:56
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.

2 participants