Zppy pcmdi 202505 rev #732
Conversation
forsyth2
added
semver: new feature
forsyth2
force-pushed
the
zppy_pcmdi_202505_rev
branch
2 times, most recently
from
f9944a9 to
3c39f69
Compare
| ref_final_yr = 1986 | ||
| ref_start_yr = 1985 | ||
| ref_years = "1985-1986", | ||
| vars = "psl" |
There was a problem hiding this comment.
@zhangshixuan1987 I'm almost done with my additions; I just need to get the integration test added. This subtask seems to be failing on the output from /lcrc/group/e3sm2/ac.wlin//E3SMv3/v3.LR.historical_0051 though. As far as I can tell, that's because psl presumably wasn't captured for that. E.g., /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/unique_id_pcmdi_diags_20250924_try5/v3.LR.historical_0051/post/atm/180x360_aave/cmip_ts/monthly doesn't have any psl nc files in it.
So a couple questions:
- Do some simulation outputs just not have the variables needed for atm variability modes? I try to integration testing for all the zppy tasks on the same simulation output. Alternatively, have I simply entered bad parameters?
- It looks like
synthetic_plotsonly allowsvariability_modesin totality -- that is, even ifvariability_modes_cplworks, ifatmfails, thensynthetic_plotscan't complete either. Is that correct?
There was a problem hiding this comment.
Hi Ryan, below is my response to the two questions:
-
This is unlikely to be the case. PSL is a standard output variable in E3SM, and I am confident it is included in the output from v3.LR.historical_0051. For reference, see my PCMDI run for v3.LR.historical_0051 at
/lcrc/group/e3sm/ac.szhang/acme_scratch/e3sm_project/test_zppy_pmp/v3.LR.historical_0051/post/atm/180x360_aave/cmip_ts/monthly. To my understanding, if the file psl.nc is missing in the cmip_ts/monthly directory, it most likely indicates that the e3sm_to_cmip conversion did not run successfully. -
This is correct. In PCMDI, variability_modes is not subdivided into variability_modes_cpl and variability_modes_atm. I introduced this split in zppy-pcmdi to enable parallel processing, since the coupled modes (variability_modes_cpl) rely on surface temperature (ts), whereas the atmospheric modes (variability_modes_atm) rely on sea level pressure (psl). The synthetic plots, however, require data from both variability_modes_cpl and variability_modes_atm to be combined.
There was a problem hiding this comment.
it most likely indicates that the e3sm_to_cmip conversion did not run successfully.
Ah yes, psl isn't included by default in vars for e3sm_to_cmip. Re-testing now.
|
@zhangshixuan1987 @chengzhuzhang I added quite a few updates to the code. I haven't had a chance to do a visual inspection of the pull requests myself, but in the interest of time, I think it would be best for us all to review the code concurrently. I'm a little concerned some of the code is hard-coded to expect specific things. For example:
But again, in the interest of time, I think it would be best to try to get this merged ASAP and then add more flexibility in the following Unified release. Here are links to specific commit blocks to review:
@chengzhuzhang For awareness, I have a bit of an ambitious to-do list for the Unified deadline of October 1 (not necessarily in this order):
|
|
Hi @forsyth2 and @chengzhuzhang : I quickly put some comments here as background information for the three questions you raised here:
**This design choice reflects the differing requirements across the three sets of diagnostics. For example, climatological means can often be estimated with 30 years—or even shorter periods—while still yielding a reasonable comparison. By contrast, robust ENSO analysis generally requires at least 30 years, and preferably longer. For modes of variability, a minimum of 50–100 years of data is often necessary to obtain reliable results. Therefore, applying a unified period across all three diagnostic sets would not be appropriate. This is the primary motivation for defining three separate sets: so that users are explicitly aware of the intended analysis timescales. However, the current implementation of figure_sets_period could be improved. A more flexible approach would be to reference the diagnostic section itself and automatically collect the period lengths specified for each category, ensuring consistency while preserving the tailored requirements of each diagnostic.**
This part is a weakness at this moment, but can be improved. One solution was to adapt the e3sm_diag function to come up a more robust viewer page for pcmdi |
examples/post.v3.LR.amip.0101.cfg
Outdated
| output = /lcrc/group/e3sm/ac.forsyth2/zppy_pr719_output/unique_id_51/v3.LR.amip_0101 | ||
| www = /lcrc/group/e3sm/public_html/diagnostic_output/ac.forsyth2/zppy_pr719_output/unique_id_51 |
There was a problem hiding this comment.
@zhangshixuan1987 If you're running the example cfg yourself, you need to change output and www accordingly.
It looks like my /lcrc/group/e3sm/ac.forsyth2/zppy_pr719_output/unique_id_51/v3.LR.amip_0101/post/scripts directory got some of your output:
cd /lcrc/group/e3sm/ac.forsyth2/zppy_pr719_output/unique_id_51/v3.LR.amip_0101/post/scripts
ls -lt pcmdi_diags_*.o*gives:
-rw-rw----+ 1 ac.szhang E3SM 944692 Sep 26 01:43 pcmdi_diags_mean_climate_model_vs_obs_2005-2006.o920507
-rw-rw----+ 1 ac.szhang E3SM 68823 Sep 26 01:43 pcmdi_diags_variability_modes_atm_model_vs_obs_2005-2006.o920514
-rw-rw----+ 1 ac.szhang E3SM 49895 Sep 26 01:42 pcmdi_diags_variability_modes_cpl_model_vs_obs_2005-2006.o920513
-rw-rw----+ 1 ac.szhang E3SM 68822 Sep 26 01:40 pcmdi_diags_variability_modes_atm_model_vs_obs_2005-2006.o920509
-rw-rw----+ 1 ac.szhang E3SM 49894 Sep 26 01:40 pcmdi_diags_variability_modes_cpl_model_vs_obs_2005-2006.o920508
-rw-rw----+ 1 ac.forsyth2 E3SM 4141187 Sep 25 20:51 pcmdi_diags_mean_climate_model_vs_obs_2005-2014.o920008
-rw-rw----+ 1 ac.forsyth2 E3SM 77567 Sep 25 20:36 pcmdi_diags_variability_modes_atm_model_vs_obs_2005-2014.o920010
-rw-rw----+ 1 ac.forsyth2 E3SM 143686 Sep 25 20:17 pcmdi_diags_variability_modes_cpl_model_vs_obs_2005-2014.o920009
Compared to an earlier run of mine:
cd /lcrc/group/e3sm/ac.forsyth2/zppy_pr719_output/unique_id_49/v3.LR.amip_0101/post/scripts
ls -lt pcmdi_diags_*.o*gives:
ls -lt pcmdi_diags_*.o*
-rw-rw----+ 1 ac.forsyth2 E3SM 262329 Sep 25 19:45 pcmdi_diags_synthetic_plots_model_vs_obs.o919961
-rw-rw----+ 1 ac.forsyth2 E3SM 4880393 Sep 25 19:43 pcmdi_diags_mean_climate_model_vs_obs_2005-2014.o919958
-rw-rw----+ 1 ac.forsyth2 E3SM 527229 Sep 25 19:29 pcmdi_diags_variability_modes_atm_model_vs_obs_2005-2014.o919960
-rw-rw----+ 1 ac.forsyth2 E3SM 593071 Sep 25 19:10 pcmdi_diags_variability_modes_cpl_model_vs_obs_2005-2014.o919959
There was a problem hiding this comment.
@forsyth2: Thanks for catching that! I had set up the test incorrectly at first, but I’ve since corrected it. I was not trying to delete the files, as I was unsure if I would delete yours.
Running full test suite
Testing processExpandlcrc_conda
# e3sm_diags
# We're just going to reuse the conda environment used on the latest test:
# https://github.com/E3SM-Project/zppy/discussions/634#discussioncomment-14556194
# e3sm-diags-main-20250926-rerun
cd ~/ez/zppy-interfaces
git status
# Check for uncommitted changes
git checkout add-pcmdi
git log
# Good, matches https://github.com/E3SM-Project/zppy-interfaces/pull/25/commits
# Good, based on top of: Add missing packages to dependencies (#32)
# Which is the latest commit on https://github.com/E3SM-Project/zppy-interfaces/commits/main
rm -rf build
conda clean --all --y # ~10 min to run
conda env create -f conda/dev.yml -n zi-pcmdi-diags-20250930
conda activate zi-pcmdi-diags-20250930
pre-commit run --all-files
python -m pip install .
pytest tests/unit/global_time_series/test_*.py
# 10 passed in 27.49s
pytest tests/unit/pcmdi_diags/test_*.py
# 7 passed, 2 warnings in 26.82s
# Comment on warnings: https://github.com/E3SM-Project/zppy-interfaces/pull/25#issuecomment-3353645549
cd ~/ez/zppy
git status
# Check for uncommitted changes
git checkout zppy_pcmdi_202505_rev
git log
# Bad, missing the latest commit from https://github.com/E3SM-Project/zppy/pull/732/commits
# Bad, based on top of: Latest NCO removes 3D vars (#717)
# Which doesn't match the latest commit on https://github.com/E3SM-Project/zppy/commits/main
#
# So, we want to get the newer commit + rebase off the latest `main`
git checkout -b zppy_pcmdi_202505_rev-backup20250930 # Make a backup
git checkout zppy_pcmdi_202505_rev
git fetch zhangshixuan1987 zppy_pcmdi_202505_rev
git reset --hard zhangshixuan1987/zppy_pcmdi_202505_rev
git log
# Good, has the new commit
# Now, to rebase off the latest `main`:
git fetch upstream
git rebase upstream/main
git log
# Good, 29 commits on top of: Integration test fixes (#737)
rm -rf build
conda clean --all --y # ~7 min to run
conda env create -f conda/dev.yml -n zppy-pcmdi-diags-20250930
conda activate zppy-pcmdi-diags-20250930
pytest tests/test_*.py
# 32 passed in 1.08s
# Edit tests/integration/utils.py:
# TEST_SPECIFICS: Dict[str, Any] = {
# "diags_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate e3sm-diags-main-20250926-rerun",
# "global_time_series_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zi-pcmdi-diags-20250930",
# "pcmdi_diags_environment_commands": "source /gpfs/fs1/home/ac.forsyth2/miniforge3/etc/profile.d/conda.sh; conda activate zi-pcmdi-diags-20250930",
# "cfgs_to_run": [
# "weekly_bundles",
# "weekly_comprehensive_v2",
# "weekly_comprehensive_v3",
# "weekly_legacy_3.0.0_bundles",
# "weekly_legacy_3.0.0_comprehensive_v2",
# "weekly_legacy_3.0.0_comprehensive_v3",
# ],
# "tasks_to_run": [
# "e3sm_diags",
# "mpas_analysis",
# "global_time_series",
# "ilamb",
# "pcmdi_diags",
# ],
# "unique_id": "test_pcmdi_diags_20250930",
# }
python tests/integration/utils.py
pre-commit run --all-files
python -m pip install .
zppy -c tests/integration/generated/test_weekly_bundles_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_comprehensive_v2_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_comprehensive_v3_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_legacy_3.0.0_bundles_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_legacy_3.0.0_comprehensive_v2_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_legacy_3.0.0_comprehensive_v3_chrysalis.cfg
# Picking up Oct. 1 ###########################################################
# Check on bundles status
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_bundles_output/test_pcmdi_diags_20250930/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.0.0_bundles_output/test_pcmdi_diags_20250930/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
# Now, run bundles part 2
cd ~/ez/zppy
# Realized later we were on branch zppy_pcmdi_zi_standalone
# But... that should be ok since we activate the correct environment:
# zppy-pcmdi-diags-20250930
# without doing a `pip install` of the current branch.
# AND the generated files should be the same since that branch didn't reset the files.
lcrc_conda
conda activate zppy-pcmdi-diags-20250930
zppy -c tests/integration/generated/test_weekly_bundles_chrysalis.cfg
zppy -c tests/integration/generated/test_weekly_legacy_3.0.0_bundles_chrysalis.cfg
# Review finished runs
### v2 ###
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_pcmdi_diags_20250930/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
# Good, no errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.0.0_comprehensive_v2_output/test_pcmdi_diags_20250930/v2.LR.historical_0201/post/scripts
grep -v "OK" *status
# Good, no errors
### v3 ###
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test_pcmdi_diags_20250930/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.0.0_comprehensive_v3_output/test_pcmdi_diags_20250930/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
### bundles ###
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_bundles_output/test_pcmdi_diags_20250930/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_legacy_3.0.0_bundles_output/test_pcmdi_diags_20250930/v3.LR.historical_0051/post/scripts
grep -v "OK" *status
# Good, no errors
# Let's proceed with the test suite to see if any more errors exist.
cd ~/ez/zppy
# MAKE SURE WE'RE ON THE RIGHT BRANCH
git status # Branch: zppy_pcmdi_zi_standalone
git add -A
git commit -m "Standalone zi testing" --no-verify
git checkout zppy_pcmdi_202505_rev
ls tests/integration/test_*.py
pytest tests/integration/test_bash_generation.py
# 1 failed in 1.90s
# Diffs expected because we introduced `environment_commands_secondary`, and added "PSL" to `vars`
pytest tests/integration/test_campaign.py
# 6 failed in 4.16s, same note as above
pytest tests/integration/test_defaults.py
# 1 failed in 1.09s, same note as above
pytest tests/integration/test_last_year.py
# 1 passed in 0.45s
pytest tests/integration/test_bundles.py
# 2 passed in 0.26s
pytest tests/integration/test_images.py
# ========================================= short test summary info =========================================
# FAILED tests/integration/test_images.py::test_images - ValueError: No images found for task pcmdi_diags in /lcrc/group/e3sm/public_html/diagnostic_output/ac....
# ====================================== 1 failed in 650.79s (0:10:50) ======================================
# Need to update tests/integration/image_checker.py:
# for task in task_list:
# if task == "pcmdi_diags":
# print(f"{task} hs no expected results yet, skipping.")
# continue
pytest tests/integration/test_images.py # ~1h to run
# Passed
cat test_images_summary.md
git diff tests/integration/image_checker.py
# diff --git a/tests/integration/image_checker.py b/tests/integration/image_checker.py
# index 53ce8fdb..f6a8da7b 100644
# --- a/tests/integration/image_checker.py
# +++ b/tests/integration/image_checker.py
# @@ -76,6 +76,9 @@ def set_up_and_run_image_checker(
# print(f"{key}: {d[key]}")
# parameters: Parameters = Parameters(d)
# for task in task_list:
# + if task == "pcmdi_diags":
# + print(f"{task} hs no expected results yet, skipping.")
# + continue
# test_results = check_images(parameters, task)
# test_results_dict[f"{cfg_specifier}_{task}"] = test_resultsLet's also review the results produced by the pcmdi task even though we don't have expected results set for them. All integration tests passed, meaning this PR isn't interfereing with the functioning of the other tasks. It also appears the PCMDI subtasks produced a working viewer. |
forsyth2
force-pushed
the
zppy_pcmdi_202505_rev
branch
from
a824086 to
d2a5982
Compare
zhangshixuan1987
force-pushed
the
zppy_pcmdi_202505_rev
branch
from
ea76241 to
a824086
Compare
|
@forsyth2 : Hi Ryan, I have finished the refactoring of both pcmdi in zppy-interface and zppy to address concerns on hard-coding in synthetic plots and viewer:
**The code changes are made in zppy through commit 1dd6157bd49d29 and commit cec3b6af0592c94 Corresponding changes in the zppy-interface are included in commit ab97e870c5a9c570 These PRs refactor the PCMDI diagnostics integration in both zppy and zppy-interface to remove hard-coded dependencies in the synthetic plots and viewer, improving flexibility and modularity. Specifically, the synthetic_plots section in .cfg files related to pcmdi is now largely independent of the other three sections. These changes are made to allow the viewer to have:
Using the updated code described above, I have tested the revised implementation with two configuration files:
The generated outputs and diagnostics can be viewed at:
I’ll ask for your review and feedback to ensure these changes align with your expectations and can be considered as a solution to your concerns. |
|
Thanks @zhangshixuan1987! I'll review & test today. |
Key changes are described below: 1. Mean Climate: revise the e3sm_to_cmip to handle the 3-D variable including U, V, T, and Z3. The "vrt_remap_plev19.nc" was added to "zppy/templates/inclusions/e3sm_to_cmip" directory to facillitate the interpolation from model level to standard pressure level 2. Modes of Variability: correction on the naming convention of the model file template so that it can be identified by the PCMDI diagnostic drivers. Also fix the flag change "plot_model = True" to "plot = True" following the changes made in the PCMDI code base 3. ENSO Diagnostics: revised the yml file to use NumPy < 2.1.0 for the PCMDI conda environment so that the “CLIVAR-PRP/ENSO_metrics” package can be called correctly for sucess diagnostics of ENSO
Added input validation to ensure all year variables are integers and within a valid order. Normalized start/end pairs so reversed inputs (e.g. y1 > y2, ref_start_yr > ref_final_yr) are automatically swapped. Recorded the original custom reference range (ref_y1, ref_y2) and clamped it to the available reference window for safe fallback. Refined the adjustment logic so: If availability fully covers the analysis period, use [y1, y2]. Otherwise, prefer the clamped custom range and trim only if it extends beyond the analysis window. If custom and analysis don’t overlap, fall back to the overlap of availability ∩ analysis. If no overlap exists, fall back to the clamped custom, and finally the full available window. Ensured final ref_y1 ≤ ref_y2 and applied zero-padding formatting once at the end.
and viewer This comment contains update in the default.ini interms of the definition of "vars" in pcmdi diagnostic sections. I now explicitly defined "clim_vars", "mova_vars", "movc_vars" and "enso_vars" to explicitly pass the needed variables for corresponding diagnostics in pcmdi. This will be later on used to process the viewer page The corresponding changes were also made in pcmdi_diags.bash to accept the new variables defined in .cfg files and default.ini
Refactored the PCMDI diagnostics workflow to strengthen viewer
robustness and configuration handling.
Key updates
------------
- default.ini:
* Added explicit defaults for viewer-related options.
* Improved parameter consistency with zppy config interface.
- pcmdi_diags.py:
* Refactored configuration parsing to ensure safe propagation of
viewer options.
* Enhanced logging for viewer setup and diagnostic dispatch.
- templates/pcmdi_diags.bash:
* Updated environment variable exports for new viewer flags.
* Improved argument quoting and error handling.
forsyth2
force-pushed
the
zppy_pcmdi_202505_rev
branch
from
c38687a to
9e399f4
Compare
2 participants
This pull request attempts to add code changes to zppy to facilitate successful zppy-PCMDI diagnostics.
Objectives:
Mean Climate: revise the e3sm_to_cmip to handle the 3-D variable, including U, V, T, and Z3. The "vrt_remap_plev19.nc" was added to "zppy/templates/inclusions/e3sm_to_cmip" directory to facilitate the interpolation from model level to standard pressure level
Modes of Variability: correction on the naming convention of the model file template so that it can be identified by the PCMDI diagnostic drivers. Also fix the flag change "plot_model = True" to "plot = True" following the changes made in the PCMDI code base
ENSO Diagnostics: revised the yml file to use NumPy < 2.1.0 for the PCMDI conda environment so that the “CLIVAR-PRP/ENSO_metrics” package can be called correctly for successful diagnostics of ENSO
Issue resolution:
This pull request is
Small Change