-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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
Update to diags handling #633
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Key Change 2: specifying a climo subsection specific to land There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
Notice,
So basically, including It only appeared like it was useful, because the land climo task often just happens to finish before the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for clarifying. I think this PR is good to go! |
||
diff_title = "Difference" | ||
ref_final_yr = 1988 | ||
ref_name = "#expand case_name#" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -110,14 +110,15 @@ ref_final_yr = 1986 | |
ref_start_yr = 1985 | ||
ref_years = "1985-1986", | ||
# Include all sets | ||
# min_case_e3sm_diags_depend_on_climo: "lat_lon","zonal_mean_xy","zonal_mean_2d","polar","cosp_histogram","meridional_mean_2d","annual_cycle_zonal_mean","zonal_mean_2d_stratosphere", | ||
# min_case_e3sm_diags_depend_on_climo: "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", | ||
# 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left |
||
short_name = "#expand case_name#" | ||
ts_daily_subsection = "atm_daily_180x360_aave" | ||
ts_num_years = 2 | ||
|
@@ -164,7 +165,7 @@ tc_obs = "#expand diagnostics_base_path#/observations/Atm/tc-analysis/" | |
|
||
[[ lnd_monthly_mvm_lnd ]] | ||
# Test model-vs-model using the same files as the reference | ||
climo_subsection = "land_monthly_climo" | ||
climo_land_subsection = "land_monthly_climo" | ||
diff_title = "Difference" | ||
partition = "#expand partition_long#" | ||
qos = "#expand qos_long#" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -533,6 +533,32 @@ def test_add_climo_dependencies(self): | |
dependencies = [] | ||
add_climo_dependencies(c, dependencies, "script_dir") | ||
self.assertEqual(dependencies, ["script_dir/climo_cdsub_1980-1990.status"]) | ||
c = {"sets": ["diurnal_cycle"]} | ||
c.update(base) | ||
dependencies = [] | ||
self.assertRaises( | ||
ParameterNotProvidedError, | ||
add_climo_dependencies, | ||
c, | ||
dependencies, | ||
"script_dir", | ||
) | ||
Comment on lines
+536
to
+545
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test that (In this unit test, we have |
||
|
||
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", | ||
) | ||
Comment on lines
+547
to
+561
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test that |
||
|
||
c = {"sets": ["tc_analysis"]} | ||
c.update(base) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
ParameterGuessType, | ||
ParameterNotProvidedError, | ||
add_dependencies, | ||
check_parameter_defined, | ||
check_required_parameters, | ||
define_or_guess, | ||
define_or_guess2, | ||
|
@@ -460,6 +461,12 @@ def test_define_or_guess2(self): | |
) | ||
self.assertEqual(c["required_parameter"], "backup_option") | ||
|
||
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") | ||
|
||
Comment on lines
+464
to
+469
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test the |
||
def test_get_file_names(self): | ||
bash, settings, status = get_file_names("script_dir", "prefix") | ||
self.assertEqual(bash, "script_dir/prefix.bash") | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,8 +6,8 @@ | |
from zppy.bundle import handle_bundles | ||
from zppy.utils import ( | ||
ParameterGuessType, | ||
ParameterNotProvidedError, | ||
add_dependencies, | ||
check_parameter_defined, | ||
check_required_parameters, | ||
check_status, | ||
define_or_guess, | ||
|
@@ -111,12 +111,9 @@ def check_parameters_for_bash(c: Dict[str, Any]) -> None: | |
|
||
def check_mvm_only_parameters_for_bash(c: Dict[str, Any]) -> None: | ||
# Check mvm-specific parameters that aren't used until e3sm_diags.bash is run. | ||
if c["diff_title"] == "": | ||
raise ParameterNotProvidedError("diff_title") | ||
if c["ref_name"] == "": | ||
raise ParameterNotProvidedError("ref_name") | ||
if c["short_ref_name"] == "": | ||
raise ParameterNotProvidedError("short_ref_name") | ||
check_parameter_defined(c, "diff_title") | ||
check_parameter_defined(c, "ref_name") | ||
check_parameter_defined(c, "short_ref_name") | ||
|
||
check_required_parameters( | ||
c, | ||
|
@@ -244,6 +241,8 @@ def add_climo_dependencies( | |
"meridional_mean_2d", | ||
"annual_cycle_zonal_mean", | ||
"zonal_mean_2d_stratosphere", | ||
"aerosol_aeronet", | ||
"aerosol_budget", | ||
Comment on lines
+244
to
+245
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Marked the aerosol sets as depending on climo. |
||
] | ||
) | ||
# Check if any requested sets depend on climo: | ||
|
@@ -256,11 +255,19 @@ def add_climo_dependencies( | |
os.path.join(script_dir, f"climo_{climo_sub}{status_suffix}"), | ||
) | ||
if "diurnal_cycle" in c["sets"]: | ||
check_parameter_defined(c, "climo_diurnal_subsection") | ||
dependencies.append( | ||
os.path.join( | ||
script_dir, f"climo_{c['climo_diurnal_subsection']}{status_suffix}" | ||
) | ||
) | ||
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}" | ||
) | ||
) | ||
if "tc_analysis" in c["sets"]: | ||
dependencies.append(os.path.join(script_dir, f"tc_analysis{status_suffix}")) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,13 +90,6 @@ cat ${result_dir}out.dat0* > ${result_dir}cyclones_${file_name}.txt | |
StitchNodes --in_fmt "lon,lat,slp,wind" --in_connect ${result_dir}connect_CSne${res}_v2.dat --range 6.0 --mintime 6 --maxgap 1 --in ${result_dir}cyclones_${file_name}.txt --out ${result_dir}cyclones_stitch_${file_name}.dat --threshold "wind,>=,17.5,6;lat,<=,40.0,6;lat,>=,-40.0,6" | ||
rm ${result_dir}cyclones_${file_name}.txt | ||
|
||
# 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 | ||
|
||
Comment on lines
-93
to
-99
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
# Generate histogram of detections | ||
HistogramNodes --in ${result_dir}cyclones_stitch_${file_name}.dat --iloncol 2 --ilatcol 3 --out ${result_dir}cyclones_hist_${file_name}.nc | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key Change 1: adding in the aerosol sets to tests.