Zppy pcmdi enhancement - #815
Conversation
|
@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:
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. |
ace60e0 to
f50333f
Compare
|
@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. |
|
@chengzhuzhang : Sure, I converted this to a draft. Please take any necessary actions and let me know if you need any help from me. |
|
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. |
1057e0f to
cd718e4
Compare
|
@zhangshixuan1987 Can you please rebase this off the latest Options to rebase: 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. |
cd718e4 to
c35d90e
Compare
|
@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! |
|
Great, thanks @zhangshixuan1987! I'll work on reviewing this PR and E3SM-Project/zppy-interfaces#49 today. |
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
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:
- Re: the rebase -- you can combine changes rather than picking one or the other. It looks like all the
vrtchanges of #827 were removed here. - Some of the error numbers need fixing, but I do like the new count-by-10s incrementing system
- 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 fortime: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_fileremoved.interp_varsdefault 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_varsdefault updated:prsnremoved,hflx→hfls. These look like correctness fixes (matching standard CMIP variable names), but should be confirmed.
3. pcmdi_diags.py: enso set handling changed
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:
-
SGS (sub-grid-scale) helper construction for
mpasseaice: Derives a static binary mask (sgs_msk_static) fromsgs_frc_static(static ice fraction), avoiding reliance ontimeMonthly_avg_icePresentIntMaskwhich may not be present in every split file. Falls back gracefully iftimeMonthly_avg_iceAreaCellis missing. -
ncremapinvocation: 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. -
Post-remap cleanup: Removes helper variables (
sgs_frc_static,sgs_msk_static, and conditionallytimeMonthly_avg_iceAreaCell) from the output, but only if they are actually present. The presence check uses agrepagainstncks -moutput. -
MPAS time metadata restoration: After remap, restores
xtime_startMonthly,xtime_endMonthly, andtimeMonthly_avg_daysSinceStartOfSimfrom the pre-remap temp file if they were dropped. -
CF-style time coordinate construction: If the output lacks a standard
timevariable, derives one fromtimeMonthly_avg_daysSinceStartOfSim, renames it, and attachesunits,calendar,long_name, andstandard_nameattributes. Usesmpas_start_timeandmpas_calendarfrom config. -
missing_valueattribute: Addsmissing_value = 1.0e36to the main floating-point variable if it is absent, without touching_FillValue. -
Cleanup: All temp files removed at the end.
Concerns:
- The
greppattern 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 usingncks --varor a more precise pattern. - Error code numbering: The existing "move output ts files" step was bumped from
ERROR (3)toERROR (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 thoughvrt_remap_varswas removed fromdefault.ini. This is either dead code or an inconsistency that needs resolution.
c35d90e to
740f231
Compare
8e1d72d to
06e5602
Compare
|
Hi @forsyth2 : I rebuilt this branch from the latest main and narrowed PR #815 to the ENSO-related changes only. The PR now enables the experimental PCMDI ENSO path, corrects the default ENSO variables, updates the documentation, and adds a regression test. The earlier MPAS, dependency-refactoring, PCMDI robustness, and error-code changes are no longer included. The vertical-remapping functionality from #827 remains intact and unchanged from main. Therefore, the earlier review threads attached to the removed changes are now outdated. The current diff is limited to four ENSO-focused files and is ready for a fresh review. |
7e14ea9 to
7d9fb73
Compare
|
Accidentally closed; reopened |
@forsyth2: Hi Ryan, based on what you responded here, I assume there is no further action needed from me regarding the PCMDI-related changes in this PR. Please let me know if I misunderstood anything or if there are any remaining issues that require additional work on my side. |
Hi @zhangshixuan1987 That's correct. Once I get #875 sorted out, I'll run a new test. |
Thank you, Ryan. |
|
@zhangshixuan1987 After a great deal of test setup modification*, it looks like we're producing results again. See here. Please let me know how that looks. Unfortunately, something went wrong with my test script, so it didn't run the tests on the output. I'm retesting** now but I won't be able to look at the results until tomorrow. * Constrained NCO to be less than 5.4.0 in Assuming the tests pass (or fail with acceptable diffs), I'll try to do visual inspections of this PR and E3SM-Project/zppy-interfaces#49. I think we're still on track to get this merged by Xylar's E3SM Unified release candidate deadline of Oct. 5, but ideally we'd get it merged much sooner. |
@forsyth2: Hi Ryan, I checked the results from the link you shared above. All of the expected results are now showing correctly, and the output looks consistent with what I expected. I think this looks good from my side. I’d also like to tag Jiwoo (@lee1043) here in case he has a chance to do a quick review of the diagnostic output. Your remaining plan also works for me. After the NCO update and the e3sm_to_cmip fix, if the new PCMDI test produces results consistent with what we are seeing here, I think it should be ready to merge. |
|
@zhangshixuan1987 Thank you for pinging me on this. I skimmed through the diagnostics in from the viewer page that was pointed from the comment in the PR. I don’t see any diagnostics that are obviously wrong, so it seems good to go! Nice organization for the results in the viewer page! This is not necessarily a technical issue from this zppy work, but during the inspection I just noticed that for ta-200 DJF, it shows strong warm bias over the polar region in the model, which might be an interest for model developers unless already known issue. |
@lee1043: Thank you, Jiwoo, for the quick review. I am glad that the current results look reasonable to you.
@lee1043: nice catch. I think this is something not simple to answer with the test here. The test here only used a short period (1985-1994), and it may not be robust enough to draw scientific conclusions. However, it seems that similar features also showed in e3sm_diag output https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.wlin/E3SMv3/v3.LR.historical_0051/e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1985-2014/viewer/lat_lon/era5/t-200mb-global-era5/ann.html We can keep monitoring this when we apply the zppy-pcmdi for the upcoming model evaluation. |
2026-09-23 Run2 zppy pcmdi_diags ENSO support testSet up and run the testSet up e3sm_to_cmip and zppy-interfaces to use pinned NCOSee #875. We can't have cd ~/ez/e3sm_to_cmip
git checkout constrain-nco
git diff HEAD^ HEAD
# nco >=5.1.4, <5.4.0
# For zppy, we've been pushing updated branches to GitHub
# so the script can fetch the latest main/master branch.
# We don't want to push a just-for-testing branch to the e3sm_to_cmip repo though.
# So, we'll make the environment ourselves.
lcrc_conda
rm -rf build
conda clean --all --y
conda env create -f conda-env/dev.yml -n test-e3sm-to-cmip-master-20260923_run2
conda activate test-e3sm-to-cmip-master-20260923_run2
pre-commit run --all-files
python -m pip install .
conda list | grep nco
# nco 5.3.9 h06dfe6a_2 conda-forge
# Good, it's not on 5.4.0cd ~/ez/zppy-interfaces
git checkout enhance_robust_pcmdi
# 34 commits from https://github.com/E3SM-Project/zppy-interfaces/pull/49/commits
git fetch upstream enhance_robust_pcmdi
git reset --hard upstream/enhance_robust_pcmdi
git checkout -b combined-pcmdi-test-20260923-run2
# 1 commit from https://github.com/E3SM-Project/zppy-interfaces/pull/62/commits
git fetch upstream nco-dev
git rebase upstream/nco-dev
git log --oneline | head -n 36
# git log --oneline | head -n 36
# ...
# 41b13dc Clude-augment enhancement on the pcmdi_diags
# 9d4e8aa Add NCO spec to conda dev file
# 477aa64 Merge pull request #61 from E3SM-Project/update-pre-commit-deps
# NOTE: For `ZI_BASE_BRANCH="combined-pcmdi-test-20260923-run2"` to work,
# we need to push that branch to GitHub so the script can fetch that branch.
# It doesn't need a PR made though.
git push upstream combined-pcmdi-test-20260923-run2Set up the zppy branchcd ~/ez/zppy
git status
# nothing to commit, working tree clean
git checkout zppy_pcmdi_enhancement
# 24 commits from https://github.com/E3SM-Project/zppy/pull/815/commits
git fetch upstream zppy_pcmdi_enhancement
git reset --hard upstream/zppy_pcmdi_enhancement
git checkout -b combined-pcmdi-test-20260923-run2
# 30 commits from https://github.com/E3SM-Project/zppy/pull/871/commits
git fetch upstream issue-869-improve-test-automation
git rebase upstream/issue-869-improve-test-automation
git log --oneline | head -n 56 # 24+30+2
# 4e8ef955 pre-commit fix
# ...
# 94fa057b Enable experimental PCMDI ENSO diagnostics
# 5735aefd One authoritative production path per case; nest development by user (#868)
# 2127adc3 Add pcmdi_diags to legacy 3.1.0 image checks
# ...
# 827cb4c6 Automate image checker, add per-task env descriptions, and auto-generate test report
# 5232fe30 Rank image check failures by severity (#865)
# Now, we have the PCMDI Diags PR (#815) built on top of the improved test script (#871)Set up the test scriptcd ~/ez/zppy
git status
# On branch combined-pcmdi-test-20260923-run2
# nothing to commit, working tree clean
# NOTE: For `ZPPY_BASE_BRANCH="combined-pcmdi-test-20260923-run2"` to work,
# we need to push that branch to GitHub so the script can fetch that branch.
# It doesn't need a PR made though.
git push upstream combined-pcmdi-test-20260923-run2
# Now, copy the test script and cfg from the zppy repo into the directory
# that you'll be running the test script from.
mkdir -p ~/ez/zppy_main_branch_tests/test_20260923_run2
cd ~/ez/zppy_main_branch_tests/test_20260923_run2
cp ~/ez/zppy/tests/main_branch_testing/run_integration_test.bash .
cp ~/ez/zppy/tests/main_branch_testing/zppy_test.cfg .
# Now, edit the test cfg as needed
emacs zppy_test.cfgSet up the test cfgls -lt /lcrc/group/e3sm/public_html/zppy_test_resources/expected_comprehensive_v3/
# Sep 14 21:11 pcmdi_diags
# Sep 4 11:55 mpas_analysis
# Sep 4 11:53 e3sm_diags
# Aug 14 17:01 global_time_series
# May 20 13:52 livvkit
# May 20 13:51 ilambshows the date results were promoted to be official expected results. We can look at the testing log to see what the run before that date was.
Run the test script# NOTE: This part is not listed in the docs, as the script should check this.
# To be on the safer side though, it's a good idea to check that these directories
# don't have uncommitted changes.
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/e3sm_to_cmip && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/e3sm_diags && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/MPAS-Analysis && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy-interfaces && git status
# nothing to commit, working tree clean
cd /lcrc/group/e3sm/ac.forsyth2/zppy_main_branch_test_dirs/zppy && git status
# Has uncommitted changes
git add -A
git commit -m "Testing" --no-verify
git status
# nothing to commit, working tree clean
cd ~/ez/zppy_main_branch_tests/test_20260923_run2
screen # Use `screen`` so that even if the terminal connection is interrupted, the script will keep running.
ulimit -s unlimited # This is necessary for MPAS-Analysis to work inside `screen`
cd ~/ez/zppy_main_branch_tests/test_20260923_run2
cat zppy_test.cfg # Make sure changes are there
time ./run_integration_test.bash --config zppy_test.cfg 2>&1 | tee integration_test_run2.log
# [2026-09-23 15:31:35] Starting zppy integration test automation
# Ctrl-A D to detach from screen
screen -ls # See what screen sessions you have
tail -f integration_test_run2.logD. Review the output# CTRL C # Exit tail
screen -R
# [2026-09-23 16:42:53] ✓ Markdown report: /home/ac.forsyth2/ez/zppy_main_branch_tests/test_20260923_run2/test_report_20260923_run2.md
# real 71m17.167s
# user 6m12.473s
# sys 1m23.184s
exit # Exit screenCopying the auto-generated AUTO-GENERATED REPORTAuto-generated report20260923 zppy testSee automated testing docs page for info on setup. Determine what the current expected results arePromotion date for each cfg/task under
Review changes since expected results were updatedCommits merged on each repo's expected-results baseline branch (see the "Branch tested" column when it differs from the branch this run actually tested) since that dependency's expected results were actually produced (for
Environment descriptionsAn
Each task's
Automated test script results
Run Python testsThe image checker (
Complete summary table
Summary table -- only failing image-check tests, sorted by task
Results analysisTODO: fill in analysis of any failures above (expected vs. unexpected, whether expected results should be updated, etc.). Manual review of results
@zhangshixuan1987 Please review these diffs. The "expected" is based off the 9/4 test run results. If these diffs look good to you, then I'll do a visual inspection of this PR and E3SM-Project/zppy-interfaces#49. If those turn out ok, we can merge.
Did ENSO plots get produced?Furthermore, the viewer includes ENSO plots now. Did any jobs fail?No, the AUTO-GENERATED REPORT notes:
|
|
@forsyth2 : Hi Ryan, thank you for running the new tests. Below is some review comments from me:
namely, we only process global region. This is not an issue, and I would recommend this default setup, while leaving the users to customize other regions like ocean and land when needed. Overall, this is good.
So, I think that this is either a misreport, or the change of the organization after the update on this PR. Therefore, I do not think these are issues.
Overall, I think that the results from this PR with the new test here are good, which indicates that the code is good to go. |
Great!
Sorry, that's what my comment 'Most are because of the typo fix in the word "pattern". we'll just have to update the expected results after merging.' was referring to.
Great! Are you able to request a Copilot review on this PR? This one won't let me, but I was able to request one on the |
I wasn't included in the E3SM project to use Copilot, so I can not issue a Copilot review. It is not clear to me why you can not issue it, as I think you have permission to use E3SM Copilot resource. Would it be possible because this is still in draft mode? I just clicked Request Review; maybe you can try to request the Copilot again and see if it works now. |
|
@zhangshixuan1987 Ah ok, it still doesn't seem to let me choose it. I can do what I did before and give the diff to Claude to comment on. I'll try to do both PR reviews tomorrow and hopefully we can merge it. Minor changes after review are probably ok, because we can always catch possible errors on the |
Another possibility is that you were not listed as an assignee. I removed the Copilot request that I submitted and added you as an assignee. Could you try requesting Copilot from your side and give it one last try? If that still does not work, your Plan B using Claude sounds good to me. Please let me know if you need any further action from my side. |
I asked it to review only for serious issues and it confirmed there's no truly serious issues to address.
Ah that seems to be it! I can request it now. Ok let me do that, thanks. I also want to do at least a high-level read-through of this PR (+1,065 -327 lines) and E3SM-Project/zppy-interfaces#49 (+2,980 -435 lines). That's a total of +4,045 -762 lines. We can proceed with this since we're so far along, but I think going forward we should try to avoid such large PRs (e.g., by splitting them up) |
| if "obs_sets" in c: | ||
| raise ValueError( | ||
| f"obs_sets is no longer supported for {c['current_set']}; " | ||
| f"use {obs_sets_parameter} instead." | ||
| ) |
There was a problem hiding this comment.
This has already been discussed and deemed acceptable since pcmdi_diags is currently a beta release. Can't seem to find the comment to link to.
There was a problem hiding this comment.
If I understand correctly, the change here is to rename obs_sets to obs_sets_parameter, right? I think this should be acceptable in our case. Since the previous beta releases have only been used by us for testing, we do not really need to maintain backward compatibility with those configurations. For the new release, users should follow the updated examples/documentation and use obs_sets_parameter, so this error should normally not be encountered.
There was a problem hiding this comment.
If I understand correctly, the change here is to rename obs_sets to obs_sets_parameter, right?
I think Copilot is saying don't error out if someone still sets obs_sets to maintain backwards compatibility. But we had already agreed it's fine to break that here.
For the new release, users should follow the updated examples/documentation and use obs_sets_parameter, so this error should normally not be encountered.
Yes, correct.
| "alternatc1" : "ceres_ebaf_v4.0", | ||
| "alternatc2" : "ceres_ebaf_v2.8", |
There was a problem hiding this comment.
As noted above, backwards compatibility is not being considered high-priority now since pcmdi_diags is a beta release.
There was a problem hiding this comment.
Thanks, Ryan. That makes sense to me. Since pcmdi_diags is still in beta and we are not prioritizing backward compatibility at this stage, I think removing the old alternate alias is fine as long as the updated examples/documentation consistently use the new aliases.
| # Sea-surface height is opt-in. Append ssh to enso_vars and default (AVISO) to | ||
| # enso_obs_sets only when an ssh model time series is available; zppy does not | ||
| # translate zos to ssh. | ||
| enso_obs_sets = string(default="alternate3,default,alternatd4,alternate3,alternate3,alternate3,alternate3,alternate3,alternate3,alternate3,alternate3,alternate3,alternate3") |
There was a problem hiding this comment.
@zhangshixuan1987 I think this comment and the next are referring to "Add adaptive num_workers safeguard for resource-intensive diagnostics." from the PR description. I think maybe that was something left over from an earlier iteration? Do we need that for this PR?
There was a problem hiding this comment.
I think this may have been left over from an earlier iteration of the PR description. In our testing so far, the PCMDI diagnostics, including ENSO, have completed successfully, and we have not encountered any issues related to num_workers. So I don't think the adaptive num_workers safeguard is necessary for this PR. We can remove that item from the PR description to keep the stated scope consistent with the implementation.
| if c["current_set"] == "enso": | ||
| logger.warning( | ||
| "The 'enso' set is not yet supported in PCMDI Diags. Skipping launching of associated jobs." | ||
| ) | ||
| break # Skip this task | ||
| logger.warning("The 'enso' set is currently experimental in PCMDI Diags.") |
There was a problem hiding this comment.
I think this is the same issue as the previous comment. The adaptive num_workers safeguard was likely left over from an earlier iteration of the PR description. In our testing so far, including the ENSO diagnostics, we have not encountered any problems related to num_workers. I therefore do not think an additional safeguard is needed for this PR, and we can remove that objective from the PR description.
The idea of adapting num_workers came from a possible performance optimization rather than from a correctness or stability issue. For mean_climate, users can select different numbers of variables; for ENSO, they can select different sets of metrics; and for modes of variability, they can choose different numbers of modes to process. Since these options are customizable, the computational workload can vary substantially. In principle, dynamically increasing or reducing num_workers based on the selected workload could improve overall efficiency by reducing both queue time and processing time. However, I think this can be treated as a separate optimization for future work rather than as a requirement for this PR.
|
@forsyth2 : Hi Ryan, see my response above, and please let me know if I misunderstood anything or if you have a different opinion. |
@forsyth2: I agree with you. As you mentioned, these PRs are large partly because this work evolved through several rounds of testing and debugging, and related changes accumulated in both zppy and zppy-interfaces as we worked through the PCMDI integration. In retrospect, we could have separated some of the functionality and fixes into smaller PRs. For this round, since the two PRs are already fairly mature and closely connected, I agree that it makes sense to proceed with them as they are. Going forward, I should definitely attempt to split larger developments into smaller, more focused PRs—for example, separating configuration/interface changes, individual diagnostic enhancements, and unrelated bug fixes where possible. That should make the review process easier and also make the purpose and testing scope of each PR clearer. |
forsyth2
left a comment
forsyth2
left a comment
There was a problem hiding this comment.
Given the size of the diffs, I've done extremely high-level visual inspections of this PR and its companion PR E3SM-Project/zppy-interfaces#49, looking for anything obviously out of place. A large part of the confidence to merge comes from the multiple iterations of both testing and Copilot/Claude reviews.
(Note Shixuan and I agreed above to try to reduce PR scope in the future.)
|
Thank you @zhangshixuan1987 for your efforts on this and the companion PR E3SM-Project/zppy-interfaces#49. It's great that we'll have ENSO supported in |
|
@forsyth2 Hi Ryan, thank you for your work on the PR. This was a great team effort, and I really appreciate your help! |
Summary
Objectives:
Issue resolution:
pcmdi_diags#807.This pull request is to:
Small Change
1. Does this do what we want it to do?
2. Are the implementation details accurate & efficient?
3. Is this well documented?
4. Is this code clean?
-Pre-commit checks: All the pre-commits checks have passed.