Zppy pcmdi enhancement

[zhangshixuan1987]

Summary

Objectives:

• Enhance robustness of the PCMDI diagnostics workflow.
• Add adaptive num_workers safeguard for resource-intensive diagnostics.
• Improve extraction and handling of observations.
• Bug fixes to NCO processing and general workflow reliability.

Issue resolution:

• No issue is reported yet. The code changes are made based on a review and some tests when attempting to resolve [Bug]: "SVD did not converge in Linear Least Squares" in pcmdi_diags #807.

This pull request is to:

• a bug fix: increment the patch version
• a small improvement: increment the minor version
• a new feature: increment the minor version
• an incompatible (non-backwards compatible) API change: increment the major version
• Please fill out either the "Small Change" or "Big Change" section (the latter includes the numbered subsections), and delete the other.

Small Change

• To merge, I will use "Squash and merge". That is, this change should be a single commit.
• Logic: I have visually inspected the entire pull request myself.
• Pre-commit checks: All the pre-commits checks have passed.

1. Does this do what we want it to do?

• I have added or modified at least one "min-case" configuration file to test this change. Every objective above is represented in at least one cfg. Note: I have tested the changes, but did not add a new or modified "min-case" config file. Please advise if this is required.

2. Are the implementation details accurate & efficient?

• I have visually inspected the entire pull request myself.
• I have left comments highlighting important pieces of code logic in the commit messages. I have had these code blocks reviewed by at least one other team member.

3. Is this well documented?

• Documentation: Key usage notes and explanations are included in the commit messages. The code changes are also commented for clarity. Please advise if further documentation updates are needed.

4. Is this code clean?

-Pre-commit checks: All the pre-commits checks have passed.

[zhangshixuan1987]

@forsyth2: I submit this pull request for some changes I made based on my observations during the testing I did when I try to resolve the reported issues for #807. I noticed in my log file for the mean climate metrics calculation:

• /compyfs/zhan391/e3sm_project/E3SMv3_testings/v3.LR.historical_0051/post/scripts/fix2/pcmdi_diags_mean_climate_model_vs_obs_1985-1994.o755476

ncra: WARNING nco_fl_lst_stdin() tried and failed to get input filename(s) from stdin ncra: ERROR received 1 positional filename(s); need at least two ncra Command line options cheatsheet (full details at http://nco.sf.net/nco.html#ncra): ncra [-3] [-4] [-5] [-6] [-7] [-A] [--bfr byt] [-C] [-c] [--cb ...] [--cmp sng] [--cnk_byt byt] [--cnk_csh byt] [--cnk_dmn nm,lmn] [--cnk_map map] [--cnk_min byt] [--cnk_plc plc] [--cnk_scl sz] [-D dbg_lvl] [-d ...] [--dbl|flt] [-F] [--fl_fmt fmt] [-G grp:lvl] [-g ...] [--gaa ...] [--gad ...] [-H] [-h] [--hdf] [--hdr_pad nbr] [--hpss] [-L lvl] [-l path] [--mro] [--msa] [-N] [-n ...] [--no_cll_msr] [--no_cll_mth] [--no_frm_trm] [--no_tmp_fl] [-O] [-o out.nc] [-p path] [--prm_ints] [--prw] [--qnt ...] [--qnt_alg alg_nm] [-R] [-r] [--ram_all] [--rec_apn] [-t thr_nbr] [--uio] [--unn] [-w wgt] [-v ...] [-X box] [-x] [-y op_typ] [-Y prg_nm] in1.nc in2.nc [...] [out.nc]

I did some debugging and found that there is a missing condition that is not fully handled in https://github.com/E3SM-Project/zppy/pull/815/changes#diff-47eb0fa6e3e0e191a714b640eddb33424a2516b9dab2c7ec57e46193d8e160bdL238-R301

Specifically, for the mean climate metrics calculation of radiative flux variables such as rsut, when CERES-EBAF is used as the observational reference, the observational dataset only covers 2001–2018. If the target model output period is, for example, 1985–1994, which is completely outside the observational coverage period, then the current code path is not able to correctly perform the observational subselection and can lead to unexpected behavior in the downstream metrics calculation. Although the metrics processing will still proceed and produce outputs, this is not the expected behavior. Instead, when this situation occurs, we would expect the workflow to fall back to using the full available observational period (2001–2018 in this case) to calculate the observational climatology for comparison, rather than attempting an invalid temporal subselection.

[chengzhuzhang]

@zhangshixuan1987 Should we convert this PR back to draft? From another thread, we decided that the pcmdi feature will remain as beta version until enso set is being finalized and integrated. We are at the end of e3sm-unified testing period, if you'd like to make more changes, we can choose to review and merge this PR after this e3sm-unified release.

[zhangshixuan1987]

@chengzhuzhang : Sure, I converted this to a draft. Please take any necessary actions and let me know if you need any help from me.

[chengzhuzhang]

Thank you @zhangshixuan1987. Let's plan to keep this PR open and integrate in the next release. Thanks for the testing and keeping enhance this feature.

[forsyth2]

@zhangshixuan1987 Can you please rebase this off the latest main? There are enough conflicts that it'd be worthwhile to rebase before I do my code review.

Options to rebase:

# Option 1:
git fetch upstream main # Assuming you named your remote "upstream", might be "origin"
git rebase upstream/main # Address conflicts here
# When I tried this, I didn't hit conflicts until 10 commits in (out of 18 added here)

# Option 2:
git fetch upstream main # Assuming you named your remote "upstream", might be "origin"
# Check what came before the 18 commits of this branch:
git log --oneline | head -n 19 # We see this branch is based off the commit 17ce9335
git rebase -i 17ce9335
# Keep "pick" for the first commit
# Use "f" for fixup for the 17 later commits
git fetch upstream main # Assuming you named your remote "upstream", might be "origin"
git rebase upstream/main
# Address conflicts here

Option 1 allows you to keep the commit history intact, but you'll have to address conflicts commit-by-commit which could be tedious. Option 2 allows you to address conflicts in one go, but it puts the work from all 18 commits under the first commit.

[zhangshixuan1987]

@forsyth2 : Hi Ryan, I did a rebased following your suggestions. Please take a look and let me know if you have any questions. Thank you!

[forsyth2]

Great, thanks @zhangshixuan1987! I'll work on reviewing this PR and E3SM-Project/zppy-interfaces#49 today.

[forsyth2]

I'm reviewing this PR and the corresponding zppy-interfaces PR together. I had Claude do an initial review (for some reason, Copilot won't let me request a review). I then did a very-high level visual inspection based on its comments. I already posted a similar review for that PR.

Once these comments are addressed, I can run zppy's integration tests on it. I'm also updating the zppy docs in #839, so I'll want to update those to reflect important changes from this PR.

A few notes:

1. Re: the rebase -- you can combine changes rather than picking one or the other. It looks like all the vrt changes of #827 were removed here.
2. Some of the error numbers need fixing, but I do like the new count-by-10s incrementing system
3. It looks like this PR enables ENSO support. Is that correct? That is, we can use ENSO after this merges?

Claude's summary

These two PRs were made in conjunction and should be reviewed together. At a high level, they extend the zppy/zppy-interfaces pipeline with MPAS ocean and sea-ice component support, while also cleaning up configuration, fixing bugs, and expanding test coverage.

---

High-Level Summary

Area	zppy	zppy-interfaces
New feature	MPAS ocean/sea-ice time-series processing	—
Config changes	New MPAS parameters; removal of vertical remap params from [ts]	Default EMOV modes list updated
Bug fixes	enso set handling; dependency wiring for e3sm_to_cmip	Typos in module names, paths, and variable names
Robustness	Multi-subsection dependency support; richer error codes in shell templates	Output dir auto-creation; Jinja2 template validation; sorted glob; normalized mode inputs
Logging	print → logger.debug in pcmdi_diags.py	print → logger.info in viewer.py
Tests	—	New tests for synthetic_metrics_plotter, expanded test_viewer.py

---

zppy PR

1. New MPAS component support (utils.py)

set_component_and_prc_typ() gains two new cases:

• mpaso → component="ocn", prc_typ="mpasocean"
• mpassi → component="ice", prc_typ="mpasseaice"

This is the foundation that enables the rest of the MPAS work.

2. New configuration parameters (default.ini)

Added to [climo]:

• mpas_rst_template — MPAS restart file template to guide time-series processing.

Added to [ts]:

• mpas_calendar (default: "noleap") — Calendar type for constructing a CF-style numeric time coordinate.
• mpas_start_time (default: "0001-01-01 00:00:00") — Reference time for time:units = "days since ...".

Removed from [ts]:

• vrt_remap_vars, vrt_remap_file, vrt_in_file — The vertical remap parameters are removed from [ts]. ⚠️ Breaking change for existing users who configured vertical remapping via these keys. Confirm whether this functionality has been moved elsewhere or intentionally dropped. The template still contains a {%- if vrt_remap_vars != '' %} block (visible at the end of the diff), suggesting some vertical remap logic may still be active — this inconsistency should be clarified.

Modified in [e3sm_to_cmip]:

• vrt_in_file removed.
• interp_vars default changed from "U,V,T,Q,RELHUM,OMEGA,Z3" to "" (empty). ⚠️ Behavioral change: previously, vertical interpolation ran by default for a standard set of 3D variables. Now users must opt in explicitly. Existing workflows that relied on the default will silently stop interpolating. This deserves prominent documentation.

Modified in [pcmdi_diags]:

• enso_vars default updated: prsn removed, hflx → hfls. These look like correctness fixes (matching standard CMIP variable names), but should be confirmed.

3. pcmdi_diags.py: enso set handling changed

⚠️ Concern. The enso set previously triggered a break to skip the task entirely, with a warning message saying it was "not yet supported." This PR changes the warning to say it is "currently a testing mode" and comments out the continue (which was itself commented out, not active). The net result is that the enso set now runs. Whether this is intentional and whether enso is actually stable enough to run by default should be confirmed with the author.

4. Multi-subsection dependency support (pcmdi_diags.py)

add_ts_dependencies() now supports comma-separated ts_subsection values, iterating over each and registering dependencies for all of them. Similarly, a new add_e3sm_to_cmip_dependencies() function was extracted and applies the same pattern for e3sm_to_cmip_atm_subsection. Both functions also defensively initialize the subsection key if it is absent before calling set_value_of_parameter_if_undefined() (which requires the key to exist). This is a solid improvement for multi-component workflows.

5. MPAS time-series shell template (large addition)

The most substantial change is a new block in the ts shell template for processing MPAS ocean and sea-ice variables. Key steps:

1. SGS (sub-grid-scale) helper construction for mpasseaice: Derives a static binary mask (sgs_msk_static) from sgs_frc_static (static ice fraction), avoiding reliance on timeMonthly_avg_icePresentIntMask which may not be present in every split file. Falls back gracefully if timeMonthly_avg_iceAreaCell is missing.

2. ncremap invocation: Two code paths — with and without SGS helpers — both using --d2f, --no_stagger, and a provided mapping file. The SGS path additionally passes --sgs_frc, --sgs_msk, and --sgs_nrm.

3. Post-remap cleanup: Removes helper variables (sgs_frc_static, sgs_msk_static, and conditionally timeMonthly_avg_iceAreaCell) from the output, but only if they are actually present. The presence check uses a grep against ncks -m output.

4. MPAS time metadata restoration: After remap, restores xtime_startMonthly, xtime_endMonthly, and timeMonthly_avg_daysSinceStartOfSim from the pre-remap temp file if they were dropped.

5. CF-style time coordinate construction: If the output lacks a standard time variable, derives one from timeMonthly_avg_daysSinceStartOfSim, renames it, and attaches units, calendar, long_name, and standard_name attributes. Uses mpas_start_time and mpas_calendar from config.

6. missing_value attribute: Adds missing_value = 1.0e36 to the main floating-point variable if it is absent, without touching _FillValue.

7. Cleanup: All temp files removed at the end.

Concerns:

• The grep pattern used to check variable presence — grep -qE "^[[:space:]]*[^:]+[[:space:]]+${main_var}(\(|\[)" — is somewhat fragile. A variable whose name is a substring of another could potentially match incorrectly. Consider using ncks --var or a more precise pattern.
• Error code numbering: The existing "move output ts files" step was bumped from ERROR (3) to ERROR (5) to make room for the new MPAS error codes (3 and 4). Ensure all error code documentation and any monitoring/alerting is updated accordingly.
• The {%- if vrt_remap_vars != '' %} block visible at the end of the diff suggests vertical remap logic is still present in the template, even though vrt_remap_vars was removed from default.ini. This is either dead code or an inconsistency that needs resolution.

[forsyth2]

From the removal of the vrt code, it looks like the rebase conflicts weren't handled correctly. This appears to be undoing the work of #827

From Claude:

---

Removing vrt_remap_vars, vrt_remap_file, and vrt_in_file from [ts] is a breaking change for any existing users configured for vertical remapping. Also, the ts shell template still appears to contain a {%- if vrt_remap_vars != '' %} block — if these keys are removed from config, that block becomes unreachable dead code (or worse, always evaluates to empty string and silently skips). Should be reconciled.

[forsyth2]

From Claude:

---

Changing the default from a standard set of 3D variables to empty means vertical interpolation is now opt-in. Any existing workflow that relied on this default will silently stop interpolating without any error or warning. This should be prominently noted in the changelog, and ideally a deprecation warning should be emitted when the old behavior would have applied.

[forsyth2]

I can update the docs (being added in #839) to reflect this.

I think we're still in a "beta release" for pcmdi_diags so it's ok to change functionality now. Ideally though, we're no longer in a prototype/beta release by the Fall E3SM Unified release deadline.

[forsyth2]

From Claude:

---

The enso set now runs instead of being skipped. A few questions: (1) Is this intentional and is enso considered stable enough for general use? (2) The commented-out continue is misleading — if the intent is to allow it to run, the comment should be removed entirely rather than left as dead code. (3) The warning message has a grammatical issue: "Turn on it with care" should be "Use with care" or "Enable with care."

[forsyth2]

This is a big change -- is this the PR that enables ENSO support for pcmdi_diags?

[forsyth2]

From Claude:

---

_as_subsection_list() was defined earlier in this file but doesn't appear to be used here — the subsection splitting is re-implemented inline with a list comprehension. Consider using the helper for consistency, or remove it if it isn't used elsewhere.

[forsyth2]

From Claude:

---

This grep pattern checks whether ${main_var} appears in ncks -m output, but it could false-positive if ${main_var} is a substring of another variable name (e.g. pr matching prsn). Consider anchoring the match more tightly or using ncks --var=${main_var} and checking the exit code instead.

[forsyth2]

Missed this in the rebase.

[forsyth2]

Repeats ERROR (3). Should be ERROR (4), exit 4

[forsyth2]

Should now be ERROR (5), exit 5

[forsyth2]

Should be # Will use error_num +1,+2,+3,+4,+5

[forsyth2]

Similarly:

create_links_acyc_climo_obs: # Will use error_num +1,+2,+3,+4,+5,+6,+7 (also note the last error line in that function doesn't add a number to error_num, so that should become the +7).

create_links_ts: # Will use error_num +1,+2,+3,+4,+5,+6

create_links_ts_obs: # Will use error_num +1,+2,+3,+4,+5,+6

[forsyth2]

✅ Clever to increment the exit codes by 10s so we can have "sub-codes" within each function. This will be much easier to maintain than keeping track of the total count.