Skip to content

Fix ilamb template for piControl year range (with leading0)#702

Merged
chengzhuzhang merged 1 commit intomainfrom
fix_ilamb_piControl_year_range
Apr 2, 2025
Merged

Fix ilamb template for piControl year range (with leading0)#702
chengzhuzhang merged 1 commit intomainfrom
fix_ilamb_piControl_year_range

Conversation

@chengzhuzhang
Copy link
Copy Markdown
Collaborator

Summary

When testing a config for piControl run, I found a bug that with year start/end with leading 0s, example "0451" to "0500", the ilamb template would mis-interprate the years and cause the run to fail.

This PR did minimal change to convert years with leading 0 by removing without other refactoring that is probably nice to have.

Issue resolution:

  • Closes #<ISSUE_NUMBER_HERE>

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 chengzhuzhang requested a review from forsyth2 April 2, 2025 20:23
Copy link
Copy Markdown
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.

This seems reasonable to me based on visual inspection. If you've tested it, then we can probably merge. If there are remaining issues, hopefully the more comprehensive pre-release integration tests will catch them.

@chengzhuzhang
Copy link
Copy Markdown
Collaborator Author

This seems reasonable to me based on visual inspection. If you've tested it, then we can probably merge. If there are remaining issues, hopefully the more comprehensive pre-release integration tests will catch them.

Sounds good. I have tested a piControl and historical case, they both created expected results. Could you approve the PR? Thanks!

@forsyth2
Copy link
Copy Markdown
Collaborator

forsyth2 commented Apr 2, 2025

Clicked approved!

@chengzhuzhang chengzhuzhang merged commit e908868 into main Apr 2, 2025
5 checks passed
@chengzhuzhang chengzhuzhang deleted the fix_ilamb_piControl_year_range branch April 2, 2025 21:10
Comment thread zppy/templates/ilamb.bash
for year in `seq $start_year {{ ts_num_years }} $end_year`;
do
start_year=`printf "%04d" ${year}`
end_year_int=$((${start_year} + {{ ts_num_years }} - 1))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@chengzhuzhang I think ${start_year} needed to be changed to ${year}. This works fine if there's only one pass of the loop, but on the second pass, it's always getting the same end year. I'm going to modify the bash script on my current run and if that passes the integration test, I'll make a PR to make that fix.

Debugging steps
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test_zppy_20250403/v3.LR.historical_0051/post/scripts/
grep -v "OK" *status
# ilamb_1985-1988.status:ERROR (1)
tail -n 1 ilamb_1985-1988.o722016 
# cp: cannot stat '/lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test_zppy_20250403/v3.LR.historical_0051/post/lnd/180x360_aave/cmip_ts/monthly//*_*_*_*_*_*_1987??-1986??.nc': No such file or directory
ls /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test_zppy_20250403/v3.LR.historical_0051/post/lnd/180x360_aave/cmip_ts/monthly/
# cLitter_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc      fHarvest_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc  mrsos_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# cLitter_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc      fHarvest_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc  mrsos_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc
# cSoilFast_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc    gpp_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc       nbp_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# cSoilFast_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc    gpp_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc       nbp_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc
# cSoilMedium_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc  lai_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc       prveg_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# cSoilMedium_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc  lai_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc       prveg_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc
# cSoilSlow_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc    mrfso_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc     ra_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# cSoilSlow_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc    mrfso_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc     ra_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc
# cVeg_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc         mrro_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc      rh_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# cVeg_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc         mrro_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc      rh_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc
# evspsblsoi_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc   mrros_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc     tran_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# evspsblsoi_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc   mrros_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc     tran_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc
# evspsblveg_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc   mrso_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc      tsl_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198501-198612.nc
# evspsblveg_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc   mrso_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc      tsl_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198701-198812.nc

cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_zppy_20250403/v2.LR.historical_0201/post/scripts/
grep -v "OK" *status
# No errors

# Why did v2 ilamb work?
ls /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_zppy_20250403/v2.LR.historical_0201/post/lnd/180x360_aave/cmip_ts/monthly/
# lai_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198001-198112.nc  lai_Lmon_E3SM-1-0_piControl_r1i1p1f1_gr_198201-198312.nc
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v2_output/test_zppy_20250403/v2.LR.historical_0201/post/scripts
grep "Copying files for" ilamb*.o*
# ilamb_1980-1981.o722048:Copying files for 1980 to 1981
# ilamb_1982-1983.o722049:Copying files for 1982 to 1983
cd /lcrc/group/e3sm/ac.forsyth2/zppy_weekly_comprehensive_v3_output/test_zppy_20250403/v3.LR.historical_0051/post/scripts/
grep "Copying files for" ilamb*.o*
# Copying files for 1985 to 1986
# Copying files for 1987 to 1986
echo 1980 | sed 's/^0*//' # 1980
echo 1981 | sed 's/^0*//' # 1981
echo 1985 | sed 's/^0*//' # 1985
echo 1988 | sed 's/^0*//' # 1988
# The issue is only on the second copying, 1987 should go to 1988, not 1986
# Let's go through the for-loop then:
for year in `seq $start_year {{ ts_num_years }} $end_year`;
do
  end_year_int=$((${start_year} + {{ ts_num_years }} - 1))
  start_year=`printf "%04d" ${year}`
  end_year=`printf "%04d" ${end_year_int}`

That would give:

year = 1985
start_year = 1985
end_year = 1985 + 2 - 1 = 1986
year = 1987
start_year = 1987
end_year = 1985 + 2 -1 = 1986

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for catching this! The fix's sounds good to me.

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