Skip to content

Remove deprecated parameters #654

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

Merged
forsyth2 merged 1 commit into from
Jan 14, 2025
Merged

Remove deprecated parameters #654

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 0 additions & 55 deletions tests/test_zppy_global_time_series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ def test_determine_components():
c: Dict[str, Any]
# Test non-legacy
c = {
"plot_names": "",
"plots_original": "",
"plots_atm": ["a"],
"plots_ice": "",
Expand All @@ -27,7 +26,6 @@ def test_determine_components():
assert c["plots_ocn"] == "None"

c = {
"plot_names": "",
"plots_original": "",
"plots_atm": "",
"plots_ice": ["a"],
Expand All @@ -45,7 +43,6 @@ def test_determine_components():
assert c["plots_ocn"] == "None"

c = {
"plot_names": "",
"plots_original": "",
"plots_atm": "",
"plots_ice": "",
Expand All @@ -63,7 +60,6 @@ def test_determine_components():
assert c["plots_ocn"] == "None"

c = {
"plot_names": "",
"plots_original": "",
"plots_atm": "",
"plots_ice": "",
Expand All @@ -80,57 +76,6 @@ def test_determine_components():
assert c["plots_lnd"] == "None"
assert c["plots_ocn"] == ["a"]

# Test legacy
base = {"plots_atm": "", "plots_ice": "", "plots_lnd": "", "plots_ocn": ""}

c = {
"plot_names": ["a"],
"plots_original": "gets_overwritten",
"atmosphere_only": False,
}
c.update(base)
determine_components(c)
assert c["plots_original"] == ["a"]
assert c["use_atm"] == True
assert c["use_ice"] == False
assert c["use_lnd"] == False
assert c["use_ocn"] == False
assert c["plots_atm"] == "None"
assert c["plots_ice"] == "None"
assert c["plots_lnd"] == "None"
assert c["plots_ocn"] == "None"

for ocn_set in ["change_ohc", "max_moc", "change_sea_level"]:
c = {
"plot_names": "",
"plots_original": [ocn_set],
"atmosphere_only": False,
}
c.update(base)
determine_components(c)
assert c["plots_original"] == [ocn_set]
assert c["use_atm"] == True
assert c["use_ice"] == False
assert c["use_lnd"] == False
assert c["use_ocn"] == True
assert c["plots_atm"] == "None"
assert c["plots_ice"] == "None"
assert c["plots_lnd"] == "None"
assert c["plots_ocn"] == "None"

c = {"plot_names": "", "plots_original": ["a"], "atmosphere_only": True}
c.update(base)
determine_components(c)
assert c["plots_original"] == ["a"]
assert c["use_atm"] == True
assert c["use_ice"] == False
assert c["use_lnd"] == False
assert c["use_ocn"] == False
assert c["plots_atm"] == "None"
assert c["plots_ice"] == "None"
assert c["plots_lnd"] == "None"
assert c["plots_ocn"] == "None"


def test_determine_and_add_dependencies():
c: Dict[str, Any]
Expand Down
14 changes: 0 additions & 14 deletions zppy/defaults/default.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,6 @@ ts_subsection = string(default="")
[tc_analysis]
# NOTE: always overrides value in [default]
input_files = string(default="eam.h2")
# TODO for v3.0.0: Remove this parameter
# DEPRECATED. No longer used.
# The scratch directory
scratch = string(default="")
Copy link
Collaborator

@chengzhuzhang chengzhuzhang Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little unsure to remove this parameter, before the test tc_analysis tests can pass on Perlmutter, let's keep your change now, and can decide latter if removing this parameter is okay, after Perlmutter change.

Copy link
Collaborator Author

@forsyth2 forsyth2 Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just the default value for scratch. The actual use of scratch was removed in the result_dir={{ scratch }}/tc-analysis_${Y1}_${Y2}/ -> result_dir={{ output }}/post/atm/tc-analysis_${Y1}_${Y2}/ change (see https://github.com/E3SM-Project/zppy/pull/632/files#r1878679038).

Are you saying 1) undo that change or 2) just hold off on removing scratch as a parameter completely until we make sure that change works on Perlmutter?

Copy link
Collaborator

@chengzhuzhang chengzhuzhang Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we can test tc_analysis on Perl-mutter to make sure, it works without needing to including intermediate output underscratch file system? If so, we are free to remove scratch parameter and the codes what supports this.

Copy link
Collaborator Author

@forsyth2 forsyth2 Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we can do this. I was holding off until the rc testing period, because I need to copy over all the test data, etc. to Perlmutter (and Compy). But I can try to do that earlier.

Copy link
Collaborator

@chengzhuzhang chengzhuzhang Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be great, thank you!

Copy link
Collaborator Author

@forsyth2 forsyth2 Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From preliminary Perlmutter testing, it looks like TC analysis is working fine without scratch, so I think this PR is good to merge.


[e3sm_diags]
# See https://e3sm-project.github.io/e3sm_diags/_build/html/master/available-parameters.html
Expand Down Expand Up @@ -300,10 +296,6 @@ stream_ocn = string(default="streams.ocean")
walltime = string(default="06:00:00")

[global_time_series]
# TODO for v3.0.0: Remove this parameter
# Deprecated; kept for backwards compatibility
# Set to True to skip figures requiring ocean data; only affects original plots
atmosphere_only = boolean(default=False)
climo_years = string_list(default=list(""))
# The color to be used for the graphs.
color = string(default="Blue")
Expand All @@ -314,12 +306,6 @@ figstr = string(default="")
# NOTE: always overrides value in [default]
input_subdir = string(default="archive/ocn/hist")
moc_file = string(default="")
# TODO for v3.0.0: Remove this parameter
# Deprecated; legacy name for plots_original; kept for backwards compatibility
# plots_original replaces it with the same default.
# So, if a cfg used the default value before, the behavior will remain the same.
# And if a cfg set plot_names explicitally, then global_time_series.py will override plots_original.
plot_names = string(default="")
# The names of the plots you want displayed
# Variable requirements:
# net_toa_flux_restom requires RESTOM
Expand Down
12 changes: 1 addition & 11 deletions zppy/global_time_series.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,29 +87,19 @@ def global_time_series(config, script_dir, existing_bundles, job_ids_file):


def determine_components(c: Dict[str, Any]) -> None:
# Handle legacy parameter
if c["plot_names"]:
logger.warning(
"warning: plot_names for global_time_series is deprecated. Setting plot_names will override the new parameter, plots_original."
)
c["plots_original"] = c["plot_names"]
# Determine which components are needed
c["use_atm"] = False
c["use_ice"] = False
c["use_lnd"] = False
c["use_ocn"] = False
if c["plots_original"]:
c["use_atm"] = True
if c["atmosphere_only"]:
logger.warning(
"warning: atmosphere_only for global_time_series is deprecated. Preferred method: remove the 3 ocean plots (change_ohc,max_moc,change_sea_level) from plots_original."
)
has_original_ocn_plots = (
("change_ohc" in c["plots_original"])
or ("max_moc" in c["plots_original"])
or ("change_sea_level" in c["plots_original"])
)
if (not c["atmosphere_only"]) and has_original_ocn_plots:
if has_original_ocn_plots:
c["use_ocn"] = True
else:
# For better string processing in global_time_series.bash
Expand Down
2 changes: 1 addition & 1 deletion zppy/templates/global_time_series.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
################################################################################
results_dir={{ prefix }}_results

zi-global-time-series --use_ocn {{ use_ocn }} --input {{ input }} --input_subdir {{ input_subdir }} --moc_file {{ moc_file }} --case_dir {{ output }} --experiment_name {{ experiment_name }} --figstr {{ figstr }} --color {{ color }} --ts_num_years {{ ts_num_years }} --plots_original {{ plots_original }} --atmosphere_only {{ atmosphere_only }} --plots_atm {{ plots_atm }} --plots_ice {{ plots_ice }} --plots_lnd {{ plots_lnd }} --plots_ocn {{ plots_ocn }} --nrows 4 --ncols 2 --results_dir ${results_dir} --regions {{ regions }} --start_yr {{ year1 }} --end_yr {{ year2 }}
zi-global-time-series --use_ocn {{ use_ocn }} --input {{ input }} --input_subdir {{ input_subdir }} --moc_file {{ moc_file }} --case_dir {{ output }} --experiment_name {{ experiment_name }} --figstr {{ figstr }} --color {{ color }} --ts_num_years {{ ts_num_years }} --plots_original {{ plots_original }} --plots_atm {{ plots_atm }} --plots_ice {{ plots_ice }} --plots_lnd {{ plots_lnd }} --plots_ocn {{ plots_ocn }} --nrows 4 --ncols 2 --results_dir ${results_dir} --regions {{ regions }} --start_yr {{ year1 }} --end_yr {{ year2 }}

echo 'Copy images to directory'
results_dir_absolute_path={{ scriptDir }}/${results_dir}
Expand Down