Skip to content

Conversation

@chengzhuzhang
Copy link
Collaborator

Summary

1.  enable streamflow enso sets in e3sm_diags  in the config
2. update to use coupled group's config for mpas-analysis and global time-series plot: use 1850 as baseline. Including comments for clarifying parameters regarding years
3. comment out `environment_commands` that activate LCRC specific e3sm_unified path, to make the .cfg more portable on other supported machines.

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.

@chengzhuzhang
Copy link
Collaborator Author

After changing ts_years to 1850 as baseline, the global_time_series job exceed wall time limit (10hr). This is the only error when I tested this new cfg and likely related to the performance issue discovered earlier with zi-global-time-series. We will have to wait to merge this PR until the performance issue is addressed.

@forsyth2
Copy link
Collaborator

Ok that makes sense; it looks like we have a chain of PRs to get through for this:

  1. [Feature]: Separate original and component plots of global_time_series zppy-interfaces#23
  2. Only read requested vars zppy-interfaces#22 && Performance improvement attempt zppy-interfaces#19
  3. This PR

@forsyth2 forsyth2 added the priority: high High priority task (for next release) label Aug 9, 2025
@chengzhuzhang
Copy link
Collaborator Author

@forsyth2 I updated this PR with the v3.1.0 features. I was considering adding PMP but decided to defer that until we transition the PMP tasks from experimental use to production. Let me know if it is reasonable.

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 Thanks, this looks good to me.

Comment on lines +4 to +6
# NOTE: This configuration generates approximately 220 tasks/jobs when fully enabled.
# Users should only enable the tasks and time periods they need based on their specific
# analysis requirements to reduce computational load and queue time.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on this, I'll just do a visual inspection.

✅ The web results appear to include all the requested plotting tasks:

output = /lcrc/group/e3sm/ac.forsyth2/zppy_example_v3/v3.LR.historical_0051
www = /lcrc/group/e3sm/public_html/diagnostic_output/ac.forsyth2/zppy_example_v3
output = /lcrc/group/e3sm/ac.zhang40/zppy_example_v3.1/v3.LR.historical_0051
www = /lcrc/group/e3sm/public_html/diagnostic_output/zppy_example/v3.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I might recommend making this v3.1.0 to match the version exactly.

@chengzhuzhang
Copy link
Collaborator Author

@forsyth2 thanks for a review! I have addressed your comment, and will merge when checks are completed.

@chengzhuzhang chengzhuzhang merged commit 7c975a1 into main Oct 24, 2025
6 checks passed
@chengzhuzhang chengzhuzhang deleted the update_example_cfg branch October 24, 2025 18:01
@forsyth2 forsyth2 mentioned this pull request Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority: high High priority task (for next release)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Updates needed for the example cfg

3 participants