Update to diags handling #633
Conversation
forsyth2
force-pushed
the
issue-622-land-checks
branch
from
9f8e681
to
c3bbb5b
Compare
|
Checked for each set in |
|
As far as I can tell, In any case, an explicit dependency check has been added with this PR. |
|
The min-case test Error description |
|
So far so good on my latest version of the cfg from #622 (comment) though: the other two diags tasks are running while the tc_analysis diags task waits for tc_analysis to finish. |
Test the cfg from #622Re: the issues with the cfg at #622 (comment). The cfg (5th iteration from the original cfg)These links show viewers:
Some sets still need better handlingLooking at the sets used in this cfg, it looks like That means those 2 sets don't occur in either the Example error from @chengzhuzhang I'll look into this more, but let me know if anything stands out as immediately wrong (either in how zppy is calling Diags or in Diags itself). Action items to do before merging this PR
|
|
@chengzhuzhang I need more context for the
|
| # min_case_e3sm_diags_depend_on_ts: "enso_diags","qbo", | ||
| # min_case_e3sm_diags_diurnal_cycle: "diurnal_cycle", | ||
| # min_case_e3sm_diags_streamflow: "streamflow", | ||
| # min_case_e3sm_diags_tc_analysis: "tc_analysis", | ||
| # min_case_e3sm_diags_tropical_subseasonal: "tropical_subseasonal", | ||
| sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","enso_diags","qbo","diurnal_cycle","streamflow","tc_analysis","tropical_subseasonal", | ||
| sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","enso_diags","qbo","diurnal_cycle","streamflow","tc_analysis","tropical_subseasonal","aerosol_aeronet","aerosol_budget" |
There was a problem hiding this comment.
All 17 sets excluding area_mean_time_series which I think isn't used much now?
Yes, this is correct
Yes, this is true
This seems to be a diags error. |
|
@chengzhuzhang I've renamed this PR to better cover that the changes affect a number of sets. We need two changes to the
Thanks! |
|
Thanks. yes, it makes sense to update the upstream tool first. |
@forsyth2 item one has been taken care of by E3SM-Project/e3sm_diags@ffbdf28. |
|
The only thing is no plots will generate for the aerosol sets if we run into that bug in the weekly testing too (now that the aerosol sets have been added to weekly testing). But correct, zppy can still function. |
The aerosl_budget table won't be created because the variables are not included in the standard v3 output that zppy weekly tests run with. @forsyth2 sorry I accidentally messed up your comment by editing... |
forsyth2
force-pushed
the
issue-622-land-checks
branch
3 times, most recently
from
527aed4
to
f30b399
Compare
forsyth2
force-pushed
the
issue-622-land-checks
branch
from
f30b399
to
cd8d349
Compare
There was a problem hiding this comment.
@chengzhuzhang This is ready for review. You can skip tests/integration/generated since those are auto-generated files. In this self-review, I've outlined 3 key changes and made other explainer comments.
| @@ -35,4 +35,4 @@ walltime = "#expand diags_walltime#" | |||
|
|
|||
| [[ atm_monthly_180x360_aave ]] | |||
| climo_subsection = "atm_monthly_180x360_aave" | |||
| sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere", | |||
| sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","aerosol_aeronet","aerosol_budget", | |||
There was a problem hiding this comment.
Key Change 1: adding in the aerosol sets to tests.
| @@ -34,7 +34,7 @@ short_name = "#expand case_name#" | |||
| walltime = "#expand diags_walltime#" | |||
|
|
|||
| [[ lnd_monthly_mvm_lnd ]] | |||
| climo_subsection = "land_monthly_climo" | |||
| climo_land_subsection = "land_monthly_climo" | |||
There was a problem hiding this comment.
Key Change 2: specifying a climo subsection specific to land
There was a problem hiding this comment.
@forsyth2 I have a clarification question regarding to backward compatibility. What would be the consequence if in an old zppy config file, a user uses climo_subsection = "land_monthly_climo"
There was a problem hiding this comment.
Ah, so that wouldn't have actually had any effect. And therefore there is nothing to be backwards compatible with.
This block is new in this PR:
if "lat_lon_land" in c["sets"]:
check_parameter_defined(c, "climo_land_subsection")
dependencies.append(
os.path.join(
script_dir, f"climo_{c['climo_land_subsection']}{status_suffix}"
)
)
Notice, lat_lon_land wasn't included in:
def add_climo_dependencies(
c: Dict[str, Any], dependencies: List[str], script_dir: str
) -> None:
depend_on_climo: Set[str] = set(
[
"lat_lon",
"zonal_mean_xy",
"zonal_mean_2d",
"polar",
"cosp_histogram",
"meridional_mean_2d",
"annual_cycle_zonal_mean",
"zonal_mean_2d_stratosphere",
"aerosol_aeronet",
"aerosol_budget",
]
)
So basically, including climo_subsection as a parameter in this block of the cfg was simply unused information before this PR.
It only appeared like it was useful, because the land climo task often just happens to finish before the lnd_monthly_mvm_lnd diags task runs anyway.
There was a problem hiding this comment.
Thanks for clarifying. I think this PR is good to go!
| # min_case_e3sm_diags_depend_on_ts: "enso_diags","qbo", | ||
| # min_case_e3sm_diags_diurnal_cycle: "diurnal_cycle", | ||
| # min_case_e3sm_diags_streamflow: "streamflow", | ||
| # min_case_e3sm_diags_tc_analysis: "tc_analysis", | ||
| # min_case_e3sm_diags_tropical_subseasonal: "tropical_subseasonal", | ||
| # TODO: Add "tc_analysis" back in after empty dat is resolved. | ||
| sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","enso_diags","qbo","diurnal_cycle","streamflow","tropical_subseasonal", | ||
| # TODO: Add "aerosol_budget" back in once that's working for v3. | ||
| sets = "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere","enso_diags","qbo","diurnal_cycle","streamflow","tropical_subseasonal","aerosol_aeronet", |
There was a problem hiding this comment.
I left "aerosol_budget" off the weekly v3 test since that won't work at the moment.
| c = {"sets": ["diurnal_cycle"]} | ||
| c.update(base) | ||
| dependencies = [] | ||
| self.assertRaises( | ||
| ParameterNotProvidedError, | ||
| add_climo_dependencies, | ||
| c, | ||
| dependencies, | ||
| "script_dir", | ||
| ) |
There was a problem hiding this comment.
Test that climo_diurnal_subsection is checked for if diurnal_cycle is requested.
(In this unit test, we have "guess_path_parameters": True, "guess_section_parameters": True, so an error will be raised if this parameter is not provided. These options are off-by-default, meaning zppy will instead try to guess the value.)
| c = {"sets": ["lat_lon_land"], "climo_land_subsection": "lndsub"} | ||
| c.update(base) | ||
| dependencies = [] | ||
| add_climo_dependencies(c, dependencies, "script_dir") | ||
| self.assertEqual(dependencies, ["script_dir/climo_lndsub_1980-1990.status"]) | ||
| c = {"sets": ["lat_lon_land"]} | ||
| c.update(base) | ||
| dependencies = [] | ||
| self.assertRaises( | ||
| ParameterNotProvidedError, | ||
| add_climo_dependencies, | ||
| c, | ||
| dependencies, | ||
| "script_dir", | ||
| ) |
There was a problem hiding this comment.
Test that climo_land_subsection is checked for if lat_lon_land is requested.
| def test_check_parameter_defined(self): | ||
| c = {"a": 1, "b": 2, "c": ""} | ||
| check_parameter_defined(c, "a") | ||
| self.assertRaises(ParameterNotProvidedError, check_parameter_defined, c, "c") | ||
| self.assertRaises(ParameterNotProvidedError, check_parameter_defined, c, "d") | ||
|
|
There was a problem hiding this comment.
Test the check_parameter_defined function.
| "aerosol_aeronet", | ||
| "aerosol_budget", |
There was a problem hiding this comment.
Marked the aerosol sets as depending on climo.
| # If cyclones_stitch file is empty, exit | ||
| if ! [ -s ${result_dir}cyclones_stitch_${file_name}.dat ]; then | ||
| cd {{ scriptDir }} | ||
| echo 'ERROR (1)' > {{ prefix }}.status | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
Key Change 3: Removed this block since this is not an error per-se, as discussed. Instead, E3SM Diags will do this check via E3SM-Project/e3sm_diags@ffbdf28
|
@forsyth2 the PR looks good, and thanks for the comments! I just had a clarification regarding the change of |
|
Thanks @chengzhuzhang! Let me know if my answer to your question is sufficient, and then I can merge this. |
Issue resolution
tc_analysisin zppy #622 (specifically the issue at [Bug]: Issues withtc_analysisin zppy #622 (comment))Select one: This pull request is...
1. Does this do what we want it to do?
Objectives:
lat_lon_landset (fixes [Bug]: Issues withtc_analysisin zppy #622 (comment))datcheck since this is not an error per-se. Instead, E3SM Diags will do this check via E3SM-Project/e3sm_diags@ffbdf28check_parameter_defined.Required:
If applicable:
2. Are the implementation details accurate & efficient?
Required:
3. Is this well documented?
Required:
default.inihas been updated. (Parameter documentation redirects to there).4. Is this code clean?
Required: