Improve Land variable handling #697
Conversation
|
@chengzhuzhang @czender I moved the land variable updates to this PR from #694, since they were beginning to get outside the scope of simply adding an example cfg. I've added three test case
Note: explicit list = "FSH,RH2M,LAISHA,LAISUN,QINTR,QOVER,QRUNOFF,QSOIL,QVEGE,QVEGT,SOILWATER_10CM,TSA,H2OSNO,TOTLITC,CWDC,SOIL1C,SOIL2C,SOIL3C,SOIL4C,WOOD_HARVESTC,TOTVEGC,NBP,GPP,AR,HR" For the last row of the table, we can see in ncap2: ERROR 1 dimensions of TLAKE_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of LAKEICEFRAC_tmp belong to template Internally generated template but 1 dimensions do not
Wed Mar 26 14:50:21 CDT 2025: Global and regional statistics output/TOTVEGN_198501_198912.nc
Wed Mar 26 14:50:21 CDT 2025: Global and regional statistics output/TKE1_198501_198912.nc
Wed Mar 26 14:50:22 CDT 2025: Global and regional statistics output/QCHARGE_198501_198912.nc
Wed Mar 26 14:50:22 CDT 2025: Global and regional statistics output/H2OSFC_198501_198912.nc
Wed Mar 26 14:50:22 CDT 2025: Global and regional statistics output/FPG_198501_198912.nc
Wed Mar 26 14:50:22 CDT 2025: Global and regional statistics output/DWT_CONV_PFLUX_GRC_198501_198912.nc
ncap2: ERROR 1 dimensions of PCT_LANDUNIT_tmp belong to template Internally generated template but 1 dimensions do not
Wed Mar 26 14:50:23 CDT 2025: Global and regional statistics output/AR_198501_198912.nc
Wed Mar 26 14:50:23 CDT 2025: Global and regional statistics output/ALTMAX_198501_198912.nc
ncap2: ERROR 1 dimensions of SOILICE_ICE_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of SOILLIQ_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of H2OSOI_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of SOILLIQ_ICE_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of TSOI_ICE_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of T_SCALAR_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of W_SCALAR_tmp belong to template Internally generated template but 1 dimensions do not
ncclimo: ERROR Failed in global and regional statistics. cmd_stt[8] failed. Debug this:
OMP_PROC_BIND=false ncap2 -h -O -s '*rgn_nbr=3;defdim("rgn",rgn_nbr);*W_SCALAR_tmp=0.0f*W_SCALAR.avg($lat,$lon);*W_SCALAR_rgn[time,rgn]=W_SCALAR_tmp;W_SCALAR_rgn@coordinates="region_name";*lat_area=lat+0.0*area;*msk_sth=0*lat_\
area.int();delete_miss(msk_sth);*msk_nrt=0*lat_area.int();delete_miss(msk_nrt);*idx_glb=0;*idx_nrt=1;*idx_sth=2;*rgn_len=19;defdim("rgn_len",rgn_len);region_name[rgn,rgn_len]=" ";region_name(idx_glb,0:5)="Global";region_name(id\
x_nrt,:)="Northern Hemisphere";region_name(idx_sth,:)="Southern Hemisphere";if(0) region_name@long_name="W_SCALAR timeseries array contains area-weighted sums over these regions"; else region_name@long_name="W_SCALAR timeseries\
array contains area-weighted averages over these regions";where(lat_area < 0.0) msk_sth=1; elsewhere msk_nrt=1;W_SCALAR_rgn(:,idx_glb)=((W_SCALAR*area*landfrac).avg($lat,$lon)/(area*landfrac).avg($lat,$lon)).float();W_SCALAR_r\
gn(:,idx_nrt)=((W_SCALAR*area*landfrac*msk_nrt).avg($lat,$lon)/(area*landfrac*msk_nrt).avg($lat,$lon)).float();W_SCALAR_rgn(:,idx_sth)=((W_SCALAR*area*landfrac*msk_sth).avg($lat,$lon)/(area*landfrac*msk_sth).avg($lat,$lon)).flo\
at();if(0){W_SCALAR_rgn(:,idx_glb)=W_SCALAR_rgn(:,idx_glb)*(area*landfrac).total($lat,$lon).float()*1.0f;W_SCALAR_rgn(:,idx_nrt)=W_SCALAR_rgn(:,idx_nrt)*(area*landfrac*msk_nrt).total($lat,$lon).float()*1.0f;W_SCALAR_rgn(:,idx_s\
th)=W_SCALAR_rgn(:,idx_sth)*(area*landfrac*msk_sth).total($lat,$lon).float()*1.0f;}W_SCALAR=W_SCALAR_rgn;push(&W_SCALAR@cell_methods," area: mean");if(exists(time_bnds)) time_bnds=time_bnds;if(exists(time_bounds)) time_bounds=t\
ime_bounds;valid_area_per_gridcell=area*landfrac;' output/W_SCALAR_198501_198912.nc output/W_SCALAR_198501_198912.nc |
|
Please respond to my last comment on #694 (comment) here not there. I think we can get this to work with the current NCO, and that I can implement a non-kludgy solution in the next NCO. |
Expand for debugging and testing stepsIn We have to edit cd ~/ez/zppy-interfaces
git status
# Check for uncommitted changes
git fetch upstream
git checkout -b fix_plots_lnd upstream/main
# Make edits to variable handling
git add -A
pre-commit run --all-files
git commit -m "Fix plots_lnd"
conda clean --all --y
conda env create -f conda/dev.yml -n zi_plots_lnd_20250326
conda activate zi_plots_lnd_20250326
pip install .
pytest tests/unit/global_time_series/*.py
# 10 passed in 24.05sThe edits in Back in the terminal window for # Make edits to variable handling
# Update tests/integration/utils.py to use dev environment for zppy-interfaces
git add -A
pre-commit run --all-files
git commit -m "Fix variable names and use newer zi"
python tests/integration/utils.py
pip install .
zppy -c tests/integration/generated/test_min_case_global_time_series_viewers_chrysalis.cfg
zppy -c tests/integration/generated/test_min_case_global_time_series_viewers_all_land_variables_chrysalis.cfg
zppy -c tests/integration/generated/test_min_case_global_time_series_viewers_undefined_variables_chrysalis.cfgResults:
So for I would assume (B), but that may take some time to figure out how to implement. As for ncap2: ERROR 1 dimensions of PCT_LANDUNIT_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of TLAKE_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of H2OSOI_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of SOILLIQ_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of T_SCALAR_tmp belong to template Internally generated template but 1 dimensions do not
ncap2: ERROR 1 dimensions of W_SCALAR_tmp belong to template Internally generated template but 1 dimensions do not
ncclimo: ERROR Failed in global and regional statistics. cmd_stt[8] failed. Debug this:
OMP_PROC_BIND=false ncap2 -h -O -s '*rgn_nbr=3;defdim("rgn",rgn_nbr);*W_SCALAR_tmp=0.0f*W_SCALAR.avg($lat,$lon);*W_SCALAR_rgn[time,rgn]=W_SCALAR_tmp;W_SCALAR_rgn@coordinates="region_name";*lat_area=lat+0.0*area;*msk_sth=0*lat_\
area.int();delete_miss(msk_sth);*msk_nrt=0*lat_area.int();delete_miss(msk_nrt);*idx_glb=0;*idx_nrt=1;*idx_sth=2;*rgn_len=19;defdim("rgn_len",rgn_len);region_name[rgn,rgn_len]=" ";region_name(idx_glb,0:5)="Global";region_name(id\
x_nrt,:)="Northern Hemisphere";region_name(idx_sth,:)="Southern Hemisphere";if(0) region_name@long_name="W_SCALAR timeseries array contains area-weighted sums over these regions"; else region_name@long_name="W_SCALAR timeseries\
array contains area-weighted averages over these regions";where(lat_area < 0.0) msk_sth=1; elsewhere msk_nrt=1;W_SCALAR_rgn(:,idx_glb)=((W_SCALAR*area*landfrac).avg($lat,$lon)/(area*landfrac).avg($lat,$lon)).float();W_SCALAR_r\
gn(:,idx_nrt)=((W_SCALAR*area*landfrac*msk_nrt).avg($lat,$lon)/(area*landfrac*msk_nrt).avg($lat,$lon)).float();W_SCALAR_rgn(:,idx_sth)=((W_SCALAR*area*landfrac*msk_sth).avg($lat,$lon)/(area*landfrac*msk_sth).avg($lat,$lon)).flo\
at();if(0){W_SCALAR_rgn(:,idx_glb)=W_SCALAR_rgn(:,idx_glb)*(area*landfrac).total($lat,$lon).float()*1.0f;W_SCALAR_rgn(:,idx_nrt)=W_SCALAR_rgn(:,idx_nrt)*(area*landfrac*msk_nrt).total($lat,$lon).float()*1.0f;W_SCALAR_rgn(:,idx_s\
th)=W_SCALAR_rgn(:,idx_sth)*(area*landfrac*msk_sth).total($lat,$lon).float()*1.0f;}W_SCALAR=W_SCALAR_rgn;push(&W_SCALAR@cell_methods," area: mean");if(exists(time_bnds)) time_bnds=time_bnds;if(exists(time_bounds)) time_bounds=t\
ime_bounds;valid_area_per_gridcell=area*landfrac;' output/W_SCALAR_198501_198912.nc output/W_SCALAR_198501_198912.nc |
|
@czender For my last comment, I used |
Could conceivably be implemented with a check if And if so, raise an exception so it will get caught at: |
I think (A) makes more sense, and should make the page easier to navigate if only a small set of variables are available. |
Ok, that is good. I think (B) would be trickier to implement than I initially thought. By the time, ExplanationTraceback (most recent call last):
File "/gpfs/fs1/home/ac.forsyth2/miniforge3/envs/zi_plots_lnd_20250326/lib/python3.12/site-packages/zppy_interfaces/global_time_series/coupled_global_plotting.py", line 609, in make_plot_pdfs
plot_generic(ax, xlim, exps, plot_name, rgn)
File "/gpfs/fs1/home/ac.forsyth2/miniforge3/envs/zi_plots_lnd_20250326/lib/python3.12/site-packages/zppy_interfaces/global_time_series/coupled_global_plotting.py", line 437, in plot_generic
plot(ax, xlim, exps, param_dict, rgn)
File "/gpfs/fs1/home/ac.forsyth2/miniforge3/envs/zi_plots_lnd_20250326/lib/python3.12/site-packages/zppy_interfaces/global_time_series/coupled_global_plotting.py", line 464, in plot
var = param_dict["var"](exp)
^^^^^^^^^^^^^^^^^^^^^^
File "/gpfs/fs1/home/ac.forsyth2/miniforge3/envs/zi_plots_lnd_20250326/lib/python3.12/site-packages/zppy_interfaces/global_time_series/coupled_global_plotting.py", line 432, in <lambda>
"var": lambda exp: np.array(exp["annual"][var_name][0]),
~~~~~~~~~~~~~^^^^^^^^^^
KeyError: 'WIND'That is, we realize "WIND" doesn't exist only once we're in the plotting function. Before that the variable extraction mechanism is an unresolved lambda function... |
|
Sorry, I didn't realize my typo; I got everything backwards. I meant to say "I would assume (A)" and "I think (A) would be trickier". |
|
Ok, this is going to take some debugging work to implement (A). |
Is the problem occurring in the plotting functionality or in the viewer component? It might be easier to debug if we isolate which part is causing the issue. |
|
Ah fantastic, the fix was actually very easy, just in a different place than I was thinking -- we can catch invalid variables much earlier on in the |
I think this issue stems from the fact than in the original 8-plot single-page PDF, we wanted to keep empty placeholder plots if ocean data was missing. Which is why the code as-is would only warn of variables that couldn't have a |
I believe that was a holdover from when we had |
|
@czender That just leaves the one remaining issue of figuring out why NCO is failing trying to exclude certain variables. Did I implement |
Maybe I didn't fully understand, but, for the original 8, I thought we want the position of the variables fairly static. But this is more of a cosmetic thing, we can review the results and then decide. |
In the original-8 format, there is only one I'm actually struggling to see how we'd get in the situation of having empty
Yes, I think all things considered, it's not worth the time to try to implement this two different ways based on if we're doing the original 8 plots vs all 354 Land variables. And frankly, it makes sense to not waste space on plots that have no data. |
|
@forsyth2 You implemented the exclusion command in accord with my instructions. However, I think my instructions that prefixing the variable names with a period would work were incorrect. Please try without the period, i.e., with |
|
@czender It turns out I had to add a few more missing variables to the exclusion list. It seems to be working now without the periods.
Can you explain this more? I'm wondering if we should allow users to specify their variable exclusion list via a parameter (and have this list as a default). I'm just not sure which variables each simulation output may contain. |
|
The exclusion list without periods will break if the input lacks any of the variables in the exclusion list. Your list is based on default ELM v0 so it will break on EAM h0 or ELM h1 default outputs. The purpose of the periods is to make each variable in the exclusion list optional in the input files. I think your previous list with the periods was breaking because it was incomplete, not because of the periods. Please retry the complete list with the periods. If it works on ELM h0, then you can add to it the multilevel fields that are in ELM h1, or EAM h0, or whatever, and nothing will break. |
@czender I'll try adding those back in too. It took a little over 2 hours to run the full @chengzhuzhang Ok it appears everything is working correctly now. Please let me know if you are happy with these Viewers and if so, I'll merge.
|
|
@forsyth2 thank you for the results. The viewer looks okay. Through I don't seam to find the original 8 panel plots. Are they just not turned on in the .cfg being tested? |
Correct, though I did actually run the |
Do you have a case both original pdf and the viewer results are available? |
@chengzhuzhang No, but I can construct one. Note that this would take another 2+ hours for
@czender No, the The line used in the bash script was: |
|
@forsyth2 I do not understand why the periods work for me and not for you/zppy. I suggest you go back to what works, and firewall that exclusion list so it is only applied when the input model is ELM. This should prevent the exclusion list from breaking the EAM timeseries. I'll work on getting ncclimo to do the right thing and select only 2D variables for timeseries to begin with. Then you can remove the kludgy exclusion list from zppy entirely after that. |
I think how it works it that, the coupled group will run the comprehensive zppy .cfg, and land group will examine the output, without needing to run zppy again on the same simulation... pinging @golaz or @wlin7 |
Well this was simple; I had set it as In any case, running a
Is that specific to Coupled runs, because Coupled by definition includes eveyrthing? I was under the impression the different groups ran their own simulations (e.g., BGC has distinct simulations from WaterCycle) that have data on variables specific to them and therefore post-processing needs specific to them. |
|
@forsyth2 and @chengzhuzhang The current NCO snapshot in my directories on Chrysalis and PM have a version of To test this with |
Thanks @czender but I'm going to have to hold off on testing that for now. We need these |
forsyth2
force-pushed
the
improve-land-vars
branch
from
20bae2d to
1fc6b20
Compare
|
@chengzhuzhang I've been having difficulty getting Chrysalis nodes all afternoon, but I finally have a Viewer with both the original plots and the land plots. Unfortunately... the original plots Viewer has no working links, but there is a PDF (one plot per page) of the 5 requested The use case of requesting both the original plots and the component plots was simply not something built into the initial implementation of the I think at this point we need to make a decision if we want to (A) raise some sort of |
|
@forsyth2 I think we really need to consult with @golaz, @wlin7 and @thorntonpe for making this decision, about the desired feature of the viewer, the expected change, and time line. I suppose this will first impact the current V3 paper and related analysis work. |
|
@forsyth2 I attempted a PR in zppy-interface (with help from Claude), does this seem reasonable to preserve the plotting feature for the original coupled 8 plots, while also support viewer for component variables? |
|
@chengzhuzhang Thanks, let me cherry-pick that commit into E3SM-Project/zppy-interfaces#16 and re-test. |
|
Also, I was trying to figure out how we missed the collision of use cases (i.e., wanting both a single-page original plots PDF and viewers for other variables). In E3SM-Project/zppy-interfaces#9 (comment) (from 1/14):
It appears the combined use case was never considered. |
|
@chengzhuzhang Ok using the code from E3SM-Project/zppy-interfaces#16 (which now includes your commit from E3SM-Project/zppy-interfaces#17) & this PR, I created this viewer. It shows I see this: So it may be a permissions issue? I will take a look. Once I get that working, then we might actually have the issue resolved. Nevertheless, I want to make sure no further specifications are missing:
|
|
I also want to highlight how this is an example of exponential complexity in zppy. Different use cases combined together (e.g., That is, a single pull request has generated 6 independent testing configurations (and there are certainly uncovered scenarios). I have been running these and looking at results manually -- for my last comment (#697 (comment)) I only tested one: I think it's a good thing that we can provide users with so much flexibility/customizability but there are certainly challenges on the design/testing side. |
Ah, https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/ac.forsyth2/zppy_min_case_global_time_series_viewers_original_atm_plus_land_www/test_pr697_20250328_try1/v3.LR.historical_0051/global_time_series/global_time_series_1985-1995_results/ links to Ok, so the link needs to be fixed |
forsyth2
force-pushed
the
improve-land-vars
branch
from
a615889 to
7e45449
Compare
|
Ok after adding my latest changes: viewer 20250328_try2 works -- |
|
Ok viewer 20250328_try4 appears to have the dimensions issue fixed. I'll run the other |
|
@chengzhuzhang This is ready for review. Unit tests pass on both zppy-interfaces and zppy. The only thing that looks a little off to me is how the original plots are linked when To review functionality (scroll right to see more of the table):
To review code changes:
Also, after the E3SM Unified release, we'll have to make a PR to incorporate @czender's NCO updates (see #697 (comment)). |
chengzhuzhang
left a comment
chengzhuzhang
left a comment
There was a problem hiding this comment.
Thank you for the progress, Ryan. All case here looks good. I'm not sure if the use case that includes all land ts var, and the original plots, is listed in this table? I think that's the configuration we will have in the example .cfg?
@chengzhuzhang Yes that's correct; that would have been a row like:
And indeed that's the example cfg in https://github.com/E3SM-Project/zppy/pull/694/files: So, in that case, I'll merge this PR, test #694 with these changes, and then merge that one too. |
Summary
Objectives:
global_time_series, in particular Land variables.cfgtest files to test different combinations of variable settings (in particularvars=""in thetstask andplots_lnd="all"in theglobal_time_seriestask)make_viewerparameter, for consistency with other boolean parameters.Issue resolution:
Select one: This pull request is...