Include ILAMB output in notebook and provide link to full diagnostic package #165
Conversation
|
Note the linked issue also requires workflow updates to include ILAMB and choose BGC or SP. |
|
|
Made some changes per discussion in CUPiD meeting today.
Still need to do the following:
|
|
The overarching score chart is not a static image-- rather an html page. Thus, I think it'd be difficult to reproduce as a static image within the CUPiD jupyterbook output. This will also be the first thing that appears when you click on the link to the full ILAMB output, so I'm going to suggest we don't implement this as one of the plots that shows up automatically within the jupyterbook after all. Also, for single runs, this is not the most interesting score chart... It's really only useful for comparisons against other runs: |
|
For display(
HTML(
'<iframe src="/path/to/table.html"></iframe>'
)
)But I agree that it's not very useful for the single run case. If the above code works, maybe we could include it in an
|
|
(my first version of the above comment relied on javascript, but I think the HTML |
|
Thanks Mike. Good point. FYI I'm still doing some final testing on making sure this runs smoothly with regridded CLM6 data.... |
|
Removing |
|
I'm still getting a VarNotInModel error from ILAMB-- also with the old configurations from the previous ILAMB example. Otherwise this full workflow is working. |
|
This worked with a different dataset: When model_setup.txt used this filepath: The exact same setup did not work with clean directories and |
| "# Model Name , Location of Files , Shift From, Shift To\n", # noqa: E501 | ||
| ) | ||
| ms.write( | ||
| f"CTSM51 , {base_case_output_dir}/lnd/hist/regrid/", |
There was a problem hiding this comment.
I should add some sort of "if regrid" then append the regrid directory here. Perhaps this would be useful to in the long term create the regridded files if they don't yet exist, and in the short term print out a "warning: please run regridding with command below"
There was a problem hiding this comment.
I added a note that regridded files are required
|
We actually want to remap with this command to avoid steradian units and instead get km^2 for area: |
There was a problem hiding this comment.
I didn't try to run this, but read through the diffs and have a few comments. Overall, this looks really good and I'm excited about adding ILAMB to external_diag_packages!
Should we address #160 in this PR, since we'll have the same problem with ILAMB and users only running CUPiD on non-land components? Seeing your change to cupid/cupid_webpage.py brought that issue to mind, but I'd be happy to wait for a later PR to try to fix it.
| if cupid_config_loc is None: | ||
| cupid_config_loc = os.path.join(cupid_root, "examples", "key_metrics") |
There was a problem hiding this comment.
Should this default be "external_diag_packages" instead of key_metrics since we won't be able to add link_to_ILAMB.ipynb in this PR?
| "# Model Name , Location of Files , Shift From, Shift To\n", # noqa: E501 | ||
| ) | ||
| ms.write( | ||
| f"CTSM51 , {base_case_output_dir}/lnd/hist/regrid/\n", |
There was a problem hiding this comment.
Instead of CTSM51, do we want this to be c_dict["global_params"]["base_case_name"]? Also, I'm surprised we are adding the baseline case but not the experimental case. Should we also have a line with c_dict["global_params"]["case_name"]?
| base_case_output_dir = os.path.join( | ||
| c_dict["global_params"]["CESM_output_dir"], | ||
| c_dict["global_params"]["base_case_name"], | ||
| ) |
There was a problem hiding this comment.
According to the spec, users can specify c_dict["global_params"]["base_case_output_dir"] if the baseline is not in the same directory as the experimental case... so I think you want something like:
if "base_case_output_dir" in c_dict["global_params"]:
base_case_output_dir = os.path.join(
c_dict["global_params"]["base_case_output_dir"],
c_dict["global_params"]["base_case_name"],
)
else:
base_case_output_dir = os.path.join(
c_dict["global_params"]["CESM_output_dir"],
c_dict["global_params"]["base_case_name"],
)| c_dict["global_params"]["CESM_output_dir"], | ||
| c_dict["global_params"]["base_case_name"], | ||
| ) | ||
| with open(cupid_config_loc + "model_setup.txt", "w") as ms: |
There was a problem hiding this comment.
this should be os.path.join(cupid_config_loc, "model_setup.txt") instead of cupid_config_loc + "model_setup.txt" -- cupid_config_loc isn't guaranteed to have the trailing /
| ms.write( | ||
| f"CTSM51 , {base_case_output_dir}/lnd/hist/regrid/\n", | ||
| ) | ||
| print(f"wrote {cupid_config_loc}model_setup.txt") |
There was a problem hiding this comment.
Same comment about using os.path.join because cupid_config_loc might not have the trailing /
| print("You can now run ILAMB with the following commands:") | ||
| print("---") | ||
| print("qinteractive -l select=1:ncpus=16:mpiprocs=16:mem=100G -l walltime=06:00:00") |
There was a problem hiding this comment.
The qinteractive is only necessary on the NCAR computers. Maybe we could do something like
print("You can now run ILAMB with the following commands:")
print("(Users on a super computer should make sure they are on a compute node rather than a login node)")
print("---")I'm not sure what to do about the fact that we're having trouble with MPI on derecho and casper; in general, users will need the mpiexec but for now it's probably okay to stick with the serial recommendation
| # - caption: Ocean | ||
| # chapters: | ||
| # - file: ocn/ocean_surface | ||
| - file: atm/link_to_ADF |
There was a problem hiding this comment.
It looks like the spacing changed here? comparing old line 195 to new line 161 I see
- - file: atm/link_to_ADF
+ - file: atm/link_to_ADF| @@ -131,6 +131,7 @@ compute_notebooks: | |||
| parameter_groups: | |||
| none: | |||
| adf_root: ../../examples/key_metrics/ADF_output/ | |||
| key_plots: ["Surface_Wind_Stress_ANN_LatLon_Vector_Mean.png", "PRECT_ANN_LatLon_Mean.png", "PS_DJF_SHPolar_Mean.png", "TaylorDiag_ANN_Special_Mean.png"] | |||
There was a problem hiding this comment.
I'm surprised these weren't already included in the config.yml, thanks for getting rid of the hard-coded list and replacing it with something configurable!
nblibrary/lnd/link_to_ILAMB.ipynb
Outdated
| " * `cd CUPiD/helper_scripts`\n", | ||
| " * `./generate_ilamb_config_file.py --cupid_file ../examples/external_diag_packages/config.yml --out_dir ../../`\n", | ||
| "4) Run ILAMB with the newly created configuration file.\n", | ||
| " * `qinteractive -l select=1:ncpus=16:mpiprocs=16:mem=100G -l walltime=06:00:00`\n", |
There was a problem hiding this comment.
I had recommended removing the qinteractive line from the helper script, but for the notebook maybe we can just make a note that this line is specific for NCAR users (and other supercomputers might have different ways to request compute resources)? Also, we can drop to ncpus=1:mpiprocs=1 while we work out issues in the environment
nblibrary/lnd/link_to_ILAMB.ipynb
Outdated
| " * `qinteractive -l select=1:ncpus=16:mpiprocs=16:mem=100G -l walltime=06:00:00`\n", | ||
| " * `conda activate cupid-analysis`\n", | ||
| " * `export ILAMB_ROOT=../../ilamb_aux`\n", | ||
| " * `mpiexec ilamb-run --config ilamb_nohoff_final_CLM.cfg --build_dir bld/ --df_errs ../../ilamb_aux/quantiles_Whittaker_cmip5v6.parquet --define_regions ../../ilamb_aux/DATA/regions/LandRegions.nc ../../ilamb_aux/DATA/regions/Whittaker.nc --regions global --model_setup model_setup.txt --filter .clm2.h0.`" |
There was a problem hiding this comment.
We should drop the mpiexec here (and add it back in once things work)
Description of changes:
model_setup.txtfile based onconfig.ymllink to ILAMBnotebook which includes a link to ILAMB full output as well as a few 'key metric' plots from ILAMB directly in the jupyterbookgladepaths) in a parallel location to ADF config file templates.pre-commitchecks passed (#8 in Adding Notebooks Guide)?