Add PCMDI Diags to zppy #647

forsyth2 · 2024-11-26T18:36:57Z

Issue resolution

Resolves Add PMP as task #478
Follow-up to Workflow for ZPPY- PCMDI for E3SM diagnostics #624, Workflow for ZPPY- PCMDI for E3SM diagnostics (with fixes on issues found in #624) #640.

Select one: This pull request is...

a bug fix: increment the patch version
a small improvement: increment the minor version
an incompatible (non-backwards compatible) API change: increment the major version

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

Objectives:

Objective 1
Objective 2
...
Objective n

Required:

Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
Testing: 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.
Testing: I have considered likely and/or severe edge cases and have included them in testing.

If applicable:

Testing: this pull request introduces an important feature or bug fix that we must test often. I have updated the weekly-test configuration files, not just a "min-case" one.
Testing: this pull request adds at least one new possible parameter to the cfg. I have tested using this parameter with and without any other parameter that may interact with it.

2. Are the implementation details accurate & efficient?

Required:

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

If applicable:

Dependencies: This pull request introduces a new dependency. I have discussed this requirement with at least one other team member. The dependency is noted in zppy/conda, not just an import statement.

3. Is this well documented?

Required:

Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.

4. Is this code clean?

Required:

Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
Pre-commit checks: All the pre-commits checks have passed.

If applicable:

Software architecture: I have discussed relevant trade-offs in design decisions with at least one other team member. It is unlikely that this pull request will increase tech debt.

forsyth2 · 2024-11-26T18:40:36Z

How I generated this PR, following up #640:

git remote -v
# Add fork to my list of remotes
git remote add zhangshixuan1987 [email protected]:zhangshixuan1987/zppy.git
git remote -v

# Fetch the branch from https://github.com/E3SM-Project/zppy/pull/640
git fetch zhangshixuan1987 zppy_pcmdi
git checkout -b zppy_pcmdi zhangshixuan1987/zppy_pcmdi

git log
# It looks like alot of "new" commits include commits on `main`
# Let's clean up the commit history, 
# so we're only looking at relevant changes in the PR

# The first commit in https://github.com/E3SM-Project/zppy/pull/640/commits is:
# Modify time series processing to include 3D model variables (9e2488cd8f14d4a9b2a016497de14096979ce4f0)
# The last commit before that was: Add PR template (#618) (7c7a8e987fab11ce6ac933e25c2880d5a66c4c6d)
# Rebase off that
git rebase -i 7c7a8e987fab11ce6ac933e25c2880d5a66c4c6d
# Pick (p) the first commit: Modify time series processing to include 3D model variables (9e2488cd8f14d4a9b2a016497de14096979ce4f0)
# Fixup (f) the rest

git log
# commit a4130d3c030b802c2b62359e74a56870472d22bf (HEAD -> zppy_pcmdi)
# Author: ShixuanZhang <[email protected]>
# Date:   Mon Sep 16 17:55:18 2024 -0500

#     Modify time series processing to include 3D model variables
    
#     The modification attemts to interpolate the 3D model level data to
#     standard pressure level specified by vrt_remap_plev19.nc. This is
#     needed by e3sm_to_cmip to convert these variables into cmip type

# commit 7c7a8e987fab11ce6ac933e25c2880d5a66c4c6d
# Author: forsyth2 <[email protected]>
# Date:   Wed Aug 21 11:09:17 2024 -0700

#     Add PR template (#618)

# Now, we can rebase off the latest `main`
git fetch upstream main
git rebase upstream/main
# We get merge conflicts
git status
# Unmerged paths:
#   (use "git restore --staged <file>..." to unstage)
#   (use "git add/rm <file>..." as appropriate to mark resolution)
# 	both modified:   conda/dev.yml
# 	both modified:   tests/integration/generated/test_min_case_add_dependencies_chrysalis.cfg
# 	both added:      tests/integration/generated/test_min_case_carryover_dependencies_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_depend_on_climo_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_depend_on_climo_mvm_2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_depend_on_ts_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_depend_on_ts_mvm_2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_diurnal_cycle_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_diurnal_cycle_mvm_2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_lat_lon_land_mvm_2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_streamflow_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_streamflow_mvm_2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_mvm_2_chrysalis.cfg
# 	both added:      tests/integration/generated/test_min_case_e3sm_diags_tc_analysis_parallel_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_min_case_e3sm_diags_tropical_subseasonal_mvm_2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_weekly_bundles_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_weekly_comprehensive_v2_chrysalis.cfg
# 	both modified:   tests/integration/generated/test_weekly_comprehensive_v3_chrysalis.cfg
# 	both modified:   tests/integration/template_weekly_comprehensive_v2.cfg
# 	both modified:   tests/integration/test_weekly.py
# 	both modified:   tests/integration/utils.py
# 	both modified:   tests/test_sections.py
# 	both added:      tests/test_zppy_e3sm_diags.py
# 	both added:      tests/test_zppy_global_time_series.py
# 	both added:      tests/test_zppy_ilamb.py
# 	both added:      tests/test_zppy_utils.py
# 	both modified:   zppy/__main__.py
# 	both modified:   zppy/defaults/default.ini
# 	both modified:   zppy/global_time_series.py
# 	deleted by us:   zppy/templates/coupled_global.py
# 	added by them:   zppy/templates/inclusions/e3sm_to_cmip/vrt_remap_plev19.nc
# 	both modified:   zppy/utils.py

# Went through all conflicted files.
# Mostly I resolved conflicts by using "current change" (i.e., what's on `main`, not what's on this PR),
# Because in most cases, the changes from this PR were erronously including the contents from commits outside the PR's scope.
# Cases where "incoming changes" (i.e., changes from the PR) were used:

# zppy/__main__.py:   existing_bundles = pcmdi_diags(config, scriptDir, existing_bundles, job_ids_file)
# => had to change scriptDir to script_dir

# zppy/defaults/default.ini: 
# cmip_metadata = string(default="e3sm_to_cmip/default_metadata.json")
# cmip_plevdata = string(default="e3sm_to_cmip/vrt_remap_plev19.nc")
# => had to change to include `inclusions/`

# Check for any remaining conflicts
git grep -n "<<<<<<< HEAD" 
# No appearances of "<<<<<<< HEAD", so there are no remaining conflicts
git add -A
git rebase --continue

# Update auto-generated files
python tests/integration/utils.py
git status
# No changes, so we updated everything in the generated/ subdir correctly.

# Let's give the commit a new name
git commit --amend

# Let's push the changes to GitHub
git push upstream zppy_pcmdi

forsyth2

Initial review, visual inspection only. I've included notes on how to split this PR up into zppy & zppy-interfaces now these have been separated out.

forsyth2 · 2024-11-26T18:41:58Z

.pre-commit-config.yaml

@@ -1,5 +1,5 @@
 exclude: "docs|node_modules|migrations|.git|.tox"
-default_stages: [commit]
+default_stages: [pre-commit]


Not sure why this changed.

forsyth2 · 2024-11-26T18:42:45Z

conda/chrisalis.yml

@@ -0,0 +1,34 @@
+name: zppy_dev


The contents of this file should be included into https://github.com/E3SM-Project/zppy-interfaces/blob/main/conda/dev.yml

forsyth2 · 2024-11-26T18:43:01Z

conda/chrisalis.yml

+  # Need to pin docutils because 0.17 has a bug with unordered lists
+  # https://github.com/readthedocs/sphinx_rtd_theme/issues/1115
+  - docutils=0.16
+prefix: /home/ac.szhang/.conda/envs/zppy_dev


There should be no dependency on any one user here.

forsyth2 · 2024-11-26T18:45:48Z

zppy/defaults/default.ini

@@ -107,9 +107,11 @@ input_component = string(default="")
 [ts]
 area_nm = string(default="area")
 cmip_metadata = string(default="inclusions/e3sm_to_cmip/default_metadata.json")
+cmip_plevdata = string(default="inclusions/e3sm_to_cmip/vrt_remap_plev19.nc")


du -sh vrt_remap_plev19.nc # 512 vrt_remap_plev19.nc du -sh default_metadata.json # 8.0K default_metadata.json

I can't tell the units of the du measurement here; I think it's just bytes. How big is this file? Less than a kilobyte? I guess that's ok, but we typically avoid including nc files in the repo.

This is pretty small, so should be ok to include in the repo -- or we could add this file to the mapping directory, for consistency.

forsyth2 · 2024-11-26T18:48:39Z

zppy/pcmdi_diags.py

@@ -0,0 +1,158 @@
+import os


I recommend taking a look at https://github.com/E3SM-Project/zppy/blob/main/zppy/e3sm_diags.py or https://github.com/E3SM-Project/zppy/blob/main/zppy/global_time_series.py for examples of how to better modularize this code.

forsyth2 · 2024-11-26T18:49:09Z

zppy/templates/coupled_global.py

@@ -0,0 +1,872 @@
+# Script to plot some global atmosphere and ocean time series


This looks like it got added back in for some reason in the rebase. Delete this; it's in zppy-interfaces now.

forsyth2 · 2024-11-26T18:51:24Z

zppy/templates/pcmdi_diags.bash

+{%- endif %}
+
+# script for pcmdi pre-processing
+cat > collect_data.py << EOF


I'm thinking this .py file should be moved over to zppy-interfaces. The goal is for the .bash file to be extremely light. Ideally, it's zppy calling some other package (e.g., e3sm_diags or zi-global-time-series, which is the global-time-series command in zppy-interfaces) with the proper arguments, and that's pretty much it.

forsyth2 · 2024-11-26T18:52:32Z

zppy/templates/pcmdi_diags/cmip_variables.json

@@ -0,0 +1,121 @@
+{


The pcmdi_diags subdirectory should be moved under inclusions/ and paths should be updated accordingly.

forsyth2 · 2024-11-26T18:53:51Z

zppy/templates/ts.bash

+    do
+      if [ -f ${file} ]; then
+        #ncks --rgr xtr_mth=mss_val --vrt_fl='{{cmip_plevdata}}' ${file} ${file}.plev
+        ncremap -p mpi --vrt_ntp=log --vrt_xtr=mss_val --vrt_out='{{cmip_plevdata}}' ${file} ${file}.plev


It looks like this block uses NCO rather than e3sm_to_cmip and should thus be moved before the change of environment ({{ e3sm_to_cmip_environment_commands }})

We should also look at implementing #467 (as a separate PR).

Include vertical remapping with the already-implemented horizontal remapping?

We should also check if vertical remapping is already covered by ncclimo

chengzhuzhang · 2024-11-26T21:39:08Z

zppy/templates/pcmdi_diags/process_sftlf.py

+import os
+import sys
+
+import cdms2 as cdm


we should try our best to not use cdms. I think PMP team is working on migrating to xarray/xcdat as well. maybe there is an updated version?

forsyth2 · 2024-11-26T21:49:37Z

If we do decide on adding a Viewer for PCMDI diags, check out E3SM-Project/zppy-interfaces#9 and #648 for seeing how global time series implements the Viewer. Ideally, we can use the same Viewer code for both global time series and PCMDI diags.

forsyth2 changed the title ~~Add PMP metrics~~ Add PMCDI Diags Nov 26, 2024

forsyth2 changed the title ~~Add PMCDI Diags~~ Add PCMDI Diags Nov 26, 2024

forsyth2 force-pushed the zppy_pcmdi branch from 4de50bb to cc4e5ad Compare November 26, 2024 18:39

Add PCMDI Diags to zppy

e7dda21

forsyth2 force-pushed the zppy_pcmdi branch from cc4e5ad to e7dda21 Compare November 26, 2024 18:39

forsyth2 changed the title ~~Add PCMDI Diags~~ Add PCMDI Diags to zppy Nov 26, 2024

forsyth2 commented Nov 26, 2024

View reviewed changes

chengzhuzhang reviewed Nov 26, 2024

View reviewed changes

forsyth2 mentioned this pull request Nov 27, 2024

Add Viewer support E3SM-Project/zppy-interfaces#9

Draft

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PCMDI Diags to zppy #647

Add PCMDI Diags to zppy #647

forsyth2 commented Nov 26, 2024

forsyth2 commented Nov 26, 2024

forsyth2 left a comment

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

forsyth2 Nov 26, 2024

chengzhuzhang Nov 26, 2024

chengzhuzhang Nov 26, 2024

forsyth2 commented Nov 26, 2024

		@@ -0,0 +1,872 @@
		# Script to plot some global atmosphere and ocean time series

Add PCMDI Diags to zppy #647

Are you sure you want to change the base?

Add PCMDI Diags to zppy #647

Conversation

forsyth2 commented Nov 26, 2024

Issue resolution

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?

forsyth2 commented Nov 26, 2024

forsyth2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forsyth2 commented Nov 26, 2024