Skip to content

Update data config for ilamb#696

Merged
chengzhuzhang merged 1 commit intomainfrom
ilamb_data_cfg_update
Mar 26, 2025
Merged

Update data config for ilamb#696
chengzhuzhang merged 1 commit intomainfrom
ilamb_data_cfg_update

Conversation

@chengzhuzhang
Copy link
Collaborator

@chengzhuzhang chengzhuzhang commented Mar 25, 2025

Summary

When going over the ilamb output with @thorntonpe , it appears that maps comparing with CERES ebaf 4.1 was not showing up in diagnostics results, even if the radiation fields are cmorized. It looks like in ilamb data config, it updated to use CERES ebaf 4.2 to replace 4.1. After updated the data config for ilamb in zppy repo. Diagnostics from most fields are created: ILAMB run example..

With this update, the only missing variables are now:
Boimass
CO2
Soil Carbon
Surface Relative Humidity

We should be able to bring in these variables, with cmorized input.

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.

@forsyth2
Copy link
Collaborator

@chengzhuzhang Is this ready for review?

@chengzhuzhang chengzhuzhang requested a review from forsyth2 March 25, 2025 23:18
@chengzhuzhang
Copy link
Collaborator Author

@chengzhuzhang Is this ready for review?

Yes, thank you!

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 Using the ilamb "min-case" cfg, I'm able to produce results with no errors in the output directory (/lcrc/group/e3sm/ac.forsyth2/zppy_min_case_ilamb_output/test_pr696_20250325/v3.LR.historical_0051/post/scripts).

Note that my results page has fewer valid results than your results, likely because the cfg you ran zppy on specified more variables (and it looks like both were run on the same simulation data).

It's possible there will be changes in expected results for integration testing due to the changes in /templates/ directory, but these can be updated in aggregate with updated results from other pre-release PRs.

Overall, I think this is good to merge if you have no further follow-ups.

@chengzhuzhang
Copy link
Collaborator Author

thank you for reviewing and testing. Yes, in my case, I included generating time series for all 2d atmospere and land variables that we can cmorize, thus generated max number of ilamb figures.

@chengzhuzhang chengzhuzhang merged commit 28da980 into main Mar 26, 2025
5 checks passed
@chengzhuzhang chengzhuzhang deleted the ilamb_data_cfg_update branch March 26, 2025 03:06
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.

2 participants