-
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
Use ref_final_yr for model vs model #668
Conversation
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.
@chengzhuzhang Ok, I think these changes look good. The unit tests and integration test cfgs will need to be updated though. I can add a commit for those changes if you'd like. Thanks.
zppy/defaults/default.ini
Outdated
@@ -171,7 +171,6 @@ output_format = string_list(default=list("png")) | |||
# See https://e3sm-project.github.io/e3sm_diags/_build/html/master/available-parameters.html | |||
output_format_subplot = string_list(default=list()) | |||
# End year (i.e., the last year to use) for the reference data | |||
# Required for "tropical_subseasonal" runs | |||
ref_end_yr = string(default="") |
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.
We can probably just remove this then.
@@ -103,7 +103,6 @@ def e3sm_diags(config: ConfigObj, script_dir: str, existing_bundles, job_ids_fil | |||
|
|||
def check_parameters_for_bash(c: Dict[str, Any]) -> None: | |||
# Check parameters that aren't used until e3sm_diags.bash is run | |||
check_required_parameters(c, set(["tropical_subseasonal"]), "ref_end_yr") | |||
check_required_parameters(c, set(["qbo"]), "ref_final_yr") |
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.
In zppy/templates/e3sm_diags.bash
, we have:
{% if run_type == "model_vs_obs" %}
# Obs
trop_param.reference_data_path = '{{ obs_ts }}'
trop_param.ref_start_yr = 2001
trop_param.ref_end_yr = 2010
Those are hard coded, so we're good to delete this check here, yes
trop_param.test_start_yr = f'{start_yr:04}' | ||
trop_param.test_end_yr = f'{end_yr:04}' |
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.
I think these are always put in as 4-digit years, so this change should be ok
@forsyth2 thanks for a review. Could you go ahead to update the tests? Thanks. |
Added a 3rd commit, will use merge commit to preserve authorship of the different commits. Unit tests pass. |
Summary
The problem might be introduced from abad674. This PR is to revert the change regarding to
ref_end_yr
:ref_final_yr
as other variability sets.Issue resolution:
Select one: This pull request is...
Please fill out either the "Small Change" or "Big Change" section (the latter includes the numbered subsections), and delete the other.
Small Change
Big Change
1. Does this do what we want it to do?
Required:
If applicable:
2. Are the implementation details accurate & efficient?
Required:
If applicable:
zppy/conda
, not just animport
statement.3. Is this well documented?
Required:
4. Is this code clean?
Required:
If applicable: