Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make ref_name optional in e3sm diags output figure names #524

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Make ref_name optional in e3sm diags output figure names #524

merged 1 commit into from
Nov 7, 2023

Conversation

forsyth2
Copy link
Collaborator

Make case optional for diags. Resolves #522.

@forsyth2 forsyth2 added the semver: small improvement Small improvement (will increment patch version) label Oct 26, 2023
@forsyth2 forsyth2 self-assigned this Oct 26, 2023
@forsyth2
Copy link
Collaborator Author 

$ git grep -n "\${case}"
templates/e3sm_diags.bash:150:create_links_climo ${climo_dir_source} ${climo_dir_primary} ${case} ${Y1} ${Y2} 1
templates/e3sm_diags.bash:167:create_links_climo_diurnal ${climo_diurnal_dir_source} ${climo_diurnal_dir_primary} ${case} ${Y1} ${Y2} 3
templates/e3sm_diags.bash:254:param.test_name = '${case}'
templates/e3sm_diags.bash:479:web_dir=${www}/${case}/e3sm_diags/{{ sub }}
templates/global_time_series.bash:112:f=${www}/${case}/global_time_series
templates/global_time_series.bash:135:rsync -a --delete ${results_dir_absolute_path} ${www}/${case}/global_time_series
templates/global_time_series.bash:144:pushd ${www}/${case}/global_time_series
templates/global_time_series.bash:152:pushd ${www}/${case}/global_time_series
templates/ilamb.bash:44:mkdir -p ${model_root}/${case}
templates/ilamb.bash:45:cd ${model_root}/${case}
templates/ilamb.bash:85:web_dir=${www}/${case}/ilamb/{{ sub }}_${Y1}-${Y2}
templates/mpas_analysis.bash:325:f=${www}/${case}/mpas_analysis/${identifier}/
templates/mpas_analysis.bash:347:rsync -a --delete ${identifier}/html/ ${www}/${case}/mpas_analysis/${identifier}/
templates/mpas_analysis.bash:355:pushd ${www}/${case}/mpas_analysis/
templates/mpas_analysis.bash:363:pushd ${www}/${case}/mpas_analysis/

@forsyth2 forsyth2 force-pushed the issue-522 branch 2 times, most recently from 53868d9 to f8e6174 Compare October 27, 2023 17:38
@forsyth2
Copy link
Collaborator Author

With the current changes:

Equivalent plots can be found at:

e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/viewer/lat_lon/gpcp_v32/prect-global-gpcp_v32/ann.html # With current changes
e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/viewer/lat_lon/gpcp_v32/prect-global-gpcp_v32/ann.html # Without these changes

# mvm files didn't generate with these changes
e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/viewer/lat_lon/model_vs_model/prect-global/ann.html # Without these changes

$ cd /lcrc/group/e3sm/ac.forsyth2/zppy_test_complete_run_output/v2.LR.historical_0201/post/scripts
$ grep -v "OK" *status
e3sm_diags_atm_monthly_180x360_aave_mvm_model_vs_model_1852-1853_vs_1850-1851.status:ERROR ()
$ cat e3sm_diags_atm_monthly_180x360_aave_mvm_model_vs_model_1852-1853_vs_1850-1851.o412745 
cp: cannot stat '/lcrc/group/e3sm/ac.forsyth2/zppy_test_complete_run_output/v2.LR.historical_0201/post/atm/180x360_aave/clim/2yr/1850_*_1851??_2??_climo.nc': No such file or directory
$ ls /lcrc/group/e3sm/ac.forsyth2/zppy_test_complete_run_output/v2.LR.historical_0201/post/atm/180x360_aave/clim/2yr/
v2.LR.historical_0201_01_185001_185101_climo.nc  v2.LR.historical_0201_07_185007_185107_climo.nc  v2.LR.historical_0201_ANN_185001_185112_climo.nc
v2.LR.historical_0201_01_185201_185301_climo.nc  v2.LR.historical_0201_07_185207_185307_climo.nc  v2.LR.historical_0201_ANN_185201_185312_climo.nc
v2.LR.historical_0201_02_185002_185102_climo.nc  v2.LR.historical_0201_08_185008_185108_climo.nc  v2.LR.historical_0201_DJF_185001_185112_climo.nc
v2.LR.historical_0201_02_185202_185302_climo.nc  v2.LR.historical_0201_08_185208_185308_climo.nc  v2.LR.historical_0201_DJF_185201_185312_climo.nc
v2.LR.historical_0201_03_185003_185103_climo.nc  v2.LR.historical_0201_09_185009_185109_climo.nc  v2.LR.historical_0201_JJA_185006_185108_climo.nc
v2.LR.historical_0201_03_185203_185303_climo.nc  v2.LR.historical_0201_09_185209_185309_climo.nc  v2.LR.historical_0201_JJA_185206_185308_climo.nc
v2.LR.historical_0201_04_185004_185104_climo.nc  v2.LR.historical_0201_10_185010_185110_climo.nc  v2.LR.historical_0201_MAM_185003_185105_climo.nc
v2.LR.historical_0201_04_185204_185304_climo.nc  v2.LR.historical_0201_10_185210_185310_climo.nc  v2.LR.historical_0201_MAM_185203_185305_climo.nc
v2.LR.historical_0201_05_185005_185105_climo.nc  v2.LR.historical_0201_11_185011_185111_climo.nc  v2.LR.historical_0201_SON_185009_185111_climo.nc
v2.LR.historical_0201_05_185205_185305_climo.nc  v2.LR.historical_0201_11_185211_185311_climo.nc  v2.LR.historical_0201_SON_185209_185311_climo.nc
v2.LR.historical_0201_06_185006_185106_climo.nc  v2.LR.historical_0201_12_185012_185112_climo.nc
v2.LR.historical_0201_06_185206_185306_climo.nc  v2.LR.historical_0201_12_185212_185312_climo.nc

That looks to me like we do still need the link names changed.

@chengzhuzhang
Copy link
Collaborator 

if [ {{ no_case_name_in_fig }} ]; then
    ref_name=""
else
    ref_name={{ ref_name }}
fi

This section of code should be after creating symlinks.

@forsyth2
Copy link
Collaborator Author

@chengzhuzhang Thanks, I changed the code accordingly.

e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/viewer/lat_lon/model_vs_model/prect-global/ann.html # With latest changes
e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/viewer/lat_lon/model_vs_model/prect-global/ann.html # Without latest changes

These still match up. Neither has the case v2.LR.historical_0201 in it though, so I guess that makes sense.

@chengzhuzhang
Copy link
Collaborator

@forsyth2 Looks better. But based on the request, the goal is to make case_name disappear as the default option for model-vs-model, i think we should have logic to handle this.
keep _mvm_case_name_in_fig = False in default.ini # For small chance that users who do want to preserve the case name in model to model case, they can set it to True.

In e3sm_diags.sh

For the e3sm_diags configuration file, add logic:

if run_type == "model-vs-model", and not `keep _mvm_case_name`
    `ref_name` =""

Let me know if it makes sense...

@forsyth2
Copy link
Collaborator Author

With latest changes (Run 3):

$ cd /lcrc/group/e3sm/ac.forsyth2/zppy_test_complete_run_output/v2.LR.historical_0201/post/scripts
$ grep -v "OK" *status
e3sm_diags_atm_monthly_180x360_aave_mvm_model_vs_model_1852-1853_vs_1850-1851.status:ERROR ()
$ cat e3sm_diags_atm_monthly_180x360_aave_mvm_model_vs_model_1852-1853_vs_1850-1851.o415539 
cp: cannot stat '/lcrc/group/e3sm/ac.forsyth2/zppy_test_complete_run_output/v2.LR.historical_0201/post/atm/180x360_aave/clim/2yr/1850_*_1851??_2??_climo.nc': No such file or directory
$ ls /lcrc/group/e3sm/ac.forsyth2/zppy_test_complete_run_output/v2.LR.historical_0201/post/atm/180x360_aave/clim/2yr/
v2.LR.historical_0201_01_185001_185101_climo.nc  v2.LR.historical_0201_07_185007_185107_climo.nc  v2.LR.historical_0201_ANN_185001_185112_climo.nc
v2.LR.historical_0201_01_185201_185301_climo.nc  v2.LR.historical_0201_07_185207_185307_climo.nc  v2.LR.historical_0201_ANN_185201_185312_climo.nc
v2.LR.historical_0201_02_185002_185102_climo.nc  v2.LR.historical_0201_08_185008_185108_climo.nc  v2.LR.historical_0201_DJF_185001_185112_climo.nc
v2.LR.historical_0201_02_185202_185302_climo.nc  v2.LR.historical_0201_08_185208_185308_climo.nc  v2.LR.historical_0201_DJF_185201_185312_climo.nc
v2.LR.historical_0201_03_185003_185103_climo.nc  v2.LR.historical_0201_09_185009_185109_climo.nc  v2.LR.historical_0201_JJA_185006_185108_climo.nc
v2.LR.historical_0201_03_185203_185303_climo.nc  v2.LR.historical_0201_09_185209_185309_climo.nc  v2.LR.historical_0201_JJA_185206_185308_climo.nc
v2.LR.historical_0201_04_185004_185104_climo.nc  v2.LR.historical_0201_10_185010_185110_climo.nc  v2.LR.historical_0201_MAM_185003_185105_climo.nc
v2.LR.historical_0201_04_185204_185304_climo.nc  v2.LR.historical_0201_10_185210_185310_climo.nc  v2.LR.historical_0201_MAM_185203_185305_climo.nc
v2.LR.historical_0201_05_185005_185105_climo.nc  v2.LR.historical_0201_11_185011_185111_climo.nc  v2.LR.historical_0201_SON_185009_185111_climo.nc
v2.LR.historical_0201_05_185205_185305_climo.nc  v2.LR.historical_0201_11_185211_185311_climo.nc  v2.LR.historical_0201_SON_185209_185311_climo.nc
v2.LR.historical_0201_06_185006_185106_climo.nc  v2.LR.historical_0201_12_185012_185112_climo.nc
v2.LR.historical_0201_06_185206_185306_climo.nc  v2.LR.historical_0201_12_185212_185312_climo.nc

The links still have the case name. I'm not sure any of the logic needeed to chang in the .py file. Going to try without that.

@chengzhuzhang
Copy link
Collaborator

I think something like below lines should be in the e3sm_diags run script, not before. Otherwise, the case name will be missing again in the links.

            if not c["keep_mvm_case_name_in_fig"]:
                c["ref_name"] = ""

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 1, 2023

Results of:

  • Run4: keep_mvm_case_name_in_fig not defined in cfg (defaults to False)
  • Run5: keep_mvm_case_name_in_fig set to False
  • Run6: keep_mvm_case_name_in_fig set to True

File paths:

Path Run4 Run5 Run6
model_vs_obs direct URL e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/lat_lon/GPCP_v3.2/GPCP_v3.2-PRECT-ANN-global.png e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/lat_lon/GPCP_v3.2/GPCP_v3.2-PRECT-ANN-global.png e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/lat_lon/GPCP_v3.2/GPCP_v3.2-PRECT-ANN-global.png
model_vs_obs viewer URL e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/viewer/lat_lon/gpcp_v32/prect-global-gpcp_v32/ann.html e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/viewer/lat_lon/gpcp_v32/prect-global-gpcp_v32/ann.html e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/viewer/lat_lon/gpcp_v32/prect-global-gpcp_v32/ann.html
model_vs_model direct URL e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/lat_lon/model_vs_model/v2.LR.historical_0201-PRECT-ANN-global.png e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/lat_lon/model_vs_model/v2.LR.historical_0201-PRECT-ANN-global.png e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/lat_lon/model_vs_model/v2.LR.historical_0201-PRECT-ANN-global.png
model_vs_model viewer URL e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/viewer/lat_lon/model_vs_model/prect-global/ann.html e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/viewer/lat_lon/model_vs_model/prect-global/ann.html e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/viewer/lat_lon/model_vs_model/prect-global/ann.html
files in post/atm/180x360_aave/clim/2yr begin with v2.LR.historical_0201? Yes Yes Yes

Ok, I now understand the request. I was looking at the viewer links, which never actually have the case in them. But the problem is with the direct links, specifically in the mvm case. So, it looks like the changes I've made so far do not fix that.

@chengzhuzhang To clarify, the below is the expected behavior, correct?

Path keep_mvm_case_name_in_fig=False (default) keep_mvm_case_name_in_fig=True
model_vs_obs direct URL does NOT include case does NOT include case
model_vs_model direct URL does NOT include case INCLUDES case
files in post/atm/180x360_aave/clim/2yr begin with case? Yes Yes

@chengzhuzhang
Copy link
Collaborator

chengzhuzhang commented Nov 2, 2023 •
edited
Loading

Thanks for working on this @forsyth2. And the table is a great idea to clarify things.
below is something I'm expecting:

Path keep_mvm_case_name_in_fig=False (default) keep_mvm_case_name_in_fig=True
model_vs_obs direct URL Always include case Always include case
model_vs_model direct URL does NOT include case INCLUDES case
files in post/atm/180x360_aave/clim/2yr begin with case? Yes Yes

So keep_mvm_case_name_in_fig only modulate model to model case, we want case name for model vs obs always. This is based on what I read from @wlin7's request.

In this case, the change should happen somewhere below:

# Prepare configuration file line from the e3sm_diags.bash

The code logic should be like something as follows: 
if model vs model and not keep_mvm_case_name_in_fig:
   ref_name = ""

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 2, 2023

Run 7: no ref_name logic. I.e., how zppy on main branch currently functions.

Path Run 7
model_vs_obs direct URL e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/lat_lon/GPCP_v3.2/GPCP_v3.2-PRECT-ANN-global.png
model_vs_model direct URL e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/lat_lon/model_vs_model/v2.LR.historical_0201-PRECT-ANN-global.png
files in post/atm/180x360_aave/clim/2yr begin with case? Yes

So, to get from current functionality to expected functionality:

  • We need to add case name to the model-vs-obs path in all circumstances.
  • We need to remove case name from the model-vs-model path if keep_mvm_case_name_in_fig=False.
  • We need to keep the links in post/atm/180x360_aave/clim/2yr including the case name.

@chengzhuzhang
Copy link
Collaborator

@forsyth2
Re: We need to add case name to the model-vs-obs path in all circumstances.
I think I need to clarify, we don't need to change anything for model-vs-obs.

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 2, 2023

I think I need to clarify, we don't need to change anything for model-vs-obs.

@chengzhuzhang Oh, that's good then, thanks. My model-vs-obs paths don't include the case-name currently (not sure if that's just my particular configuration).

@chengzhuzhang
Copy link
Collaborator

I think I need to clarify, we don't need to change anything for model-vs-obs.

@chengzhuzhang Oh, that's good then, thanks. My model-vs-obs paths don't include the case-name currently (not sure if that's just my particular configuration).

in the path, the ref_name (i.g. GPCP_v3.2) is included. In the model vs model case shown here, the ref_name should be the reference model v2.LR.historical_0201.

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 2, 2023

ref_name (i.g. GPCP_v3.2) is included

Oh ok, that makes sense! Sorry, conflicting terminology case-name vs ref-name. (I mistakenly thought we wanted the case v2.LR.historical_0201 to appear in the model-vs-obs path.)

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 3, 2023

Ok, I think I have a successful fix. Just need to check with keep_mvm_case_name_in_fig=True, clean up the pull request, run the tests, and merge.

Path Latest commit, keep_mvm_case_name_in_fig=False
model_vs_obs direct URL e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/lat_lon/GPCP_v3.2/GPCP_v3.2-PRECT-ANN-global.png
model_vs_model direct URL e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/lat_lon/model_vs_model/-PRECT-ANN-global.png
files in post/atm/180x360_aave/clim/2yr begin with case? Yes

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 3, 2023

Path Latest commit, keep_mvm_case_name_in_fig=True
model_vs_obs direct URL e3sm_diags/atm_monthly_180x360_aave/model_vs_obs_1850-1851/lat_lon/GPCP_v3.2/GPCP_v3.2-PRECT-ANN-global.png
model_vs_model direct URL e3sm_diags/atm_monthly_180x360_aave_mvm/model_vs_model_1852-1853/lat_lon/model_vs_model/v2.LR.historical_0201-PRECT-ANN-global.png
files in post/atm/180x360_aave/clim/2yr begin with case? Yes

Ok, that looks good too.

@wlin7
Copy link

wlin7 commented Nov 3, 2023

Thank you @forsyth2 and @chengzhuzhang for creating this option. The current IICE comparison viewer will work well with e3sm_diags generated with keep_mvm_case_name_in_fig=False

@chengzhuzhang
Copy link
Collaborator

chengzhuzhang commented Nov 6, 2023 •
edited
Loading

@forsyth2 I briefly checked with @wlin7,and he suggested to have the new parameter to keep ref name True as default for model vs model. This will have backward compatibility. Users who wants to use IICE can turn it to False.

@forsyth2
Copy link
Collaborator Author

forsyth2 commented Nov 6, 2023

This will have backward compatibility. Users who wants to use IICE can turn it to False.

Ah ok, good idea. I will make that change and re-run the tests. Then I can make the patch release-candidate.

@forsyth2 forsyth2 marked this pull request as ready for review November 7, 2023 01:20
@forsyth2 forsyth2 merged commit 577da15 into main Nov 7, 2023
4 checks passed
@forsyth2 forsyth2 deleted the issue-522 branch November 7, 2023 01:20
@chengzhuzhang chengzhuzhang changed the title Make case optional for diags Make ref_name optional in e3sm diags output figure names Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@forsyth2 forsyth2

Labels
semver: small improvement Small improvement (will increment patch version)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Option to not include casename in model_vs_model diagnostic plot filename
3 participants
@forsyth2 @chengzhuzhang @wlin7