Skip to content
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

Merged
merged 3 commits into from
Feb 4, 2025
Merged

Use ref_final_yr for model vs model #668

merged 3 commits into from
Feb 4, 2025

Conversation

chengzhuzhang
Copy link
Collaborator

Summary

The problem might be introduced from abad674. This PR is to revert the change regarding to ref_end_yr:

  1. revert to use ref_final_yr as other variability sets.
  2. remove checking this variable

Issue resolution:

Select one: This pull request is...

  • a bug fix: increment the patch version
  • a small improvement: increment the minor version
  • a new feature: increment the minor version
  • an incompatible (non-backwards compatible) API change: increment the major version

Please fill out either the "Small Change" or "Big Change" section (the latter includes the numbered subsections), and delete the other.

Small Change

  • To merge, I will use "Squash and merge". That is, this change should be a single commit.
  • Logic: I have visually inspected the entire pull request myself.
  • Pre-commit checks: All the pre-commits checks have passed.

Big Change

  • To merge, I will use "Create a merge commit". That is, this change is large enough to require multiple units of work (i.e., it should be multiple commits).

1. Does this do what we want it to do?

Required:

  • Product Management: I have confirmed with the stakeholders that the objectives above are correct and complete.
  • Testing: I have added or modified at least one "min-case" configuration file to test this change. Every objective above is represented in at least one cfg.
  • Testing: I have considered likely and/or severe edge cases and have included them in testing.

If applicable:

  • Testing: this pull request introduces an important feature or bug fix that we must test often. I have updated the weekly-test configuration files, not just a "min-case" one.
  • Testing: this pull request adds at least one new possible parameter to the cfg. I have tested using this parameter with and without any other parameter that may interact with it.

2. Are the implementation details accurate & efficient?

Required:

  • Logic: I have visually inspected the entire pull request myself.
  • Logic: I have left GitHub comments highlighting important pieces of code logic. I have had these code blocks reviewed by at least one other team member.

If applicable:

  • Dependencies: This pull request introduces a new dependency. I have discussed this requirement with at least one other team member. The dependency is noted in zppy/conda, not just an import statement.

3. Is this well documented?

Required:

  • Documentation: by looking at the docs, a new user could easily understand the functionality introduced by this pull request.

4. Is this code clean?

Required:

  • Readability: The code is as simple as possible and well-commented, such that a new team member could understand what's happening.
  • Pre-commit checks: All the pre-commits checks have passed.

If applicable:

  • Software architecture: I have discussed relevant trade-offs in design decisions with at least one other team member. It is unlikely that this pull request will increase tech debt.

Copy link
Collaborator

@forsyth2 forsyth2 left a 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.

@@ -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="")
Copy link
Collaborator

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")
Copy link
Collaborator

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

Comment on lines -290 to -291
trop_param.test_start_yr = f'{start_yr:04}'
trop_param.test_end_yr = f'{end_yr:04}'
Copy link
Collaborator

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

@chengzhuzhang
Copy link
Collaborator Author

@forsyth2 thanks for a review. Could you go ahead to update the tests? Thanks.

@forsyth2
Copy link
Collaborator

forsyth2 commented Feb 4, 2025

Added a 3rd commit, will use merge commit to preserve authorship of the different commits. Unit tests pass.

@forsyth2 forsyth2 marked this pull request as ready for review February 4, 2025 00:45
@forsyth2 forsyth2 merged commit 1f4dd5e into main Feb 4, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Error from parameter checking: raise ParameterNotProvidedError(relevant_parameter)
2 participants