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.

Sign up for GitHub

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

Jump to bottom

Fixes the annual C budget in ELM #6405

Merged
merged 6 commits into from
May 23, 2024
Merged

Fixes the annual C budget in ELM #6405

merged 6 commits into from
May 23, 2024

Conversation

bishtgautam
Copy link
Contributor

@bishtgautam bishtgautam commented May 9, 2024 •
edited by peterdschwartz
Loading

  • The C-states for all budgets (daily/monthly/annual/instantaneous/all time)
    are updated on the first ELM step.
  • Adds a 5-day SMS test that starts on 0001-12-28 and runs till 0002-01-01.

Fixes #6337
[non-BFB] for tests greater than a year with budgets.

@bishtgautam
Fixes the annual C budget in ELM
9f14bdc 
The C-states for all budgets (daily/monthly/annual/instantaneous/all time)
are updated on the first ELM step.

Fixes #6337
[BFB]
@bishtgautam bishtgautam force-pushed the bishtgautam/lnd/fixes-cnp branch from d1a7353 to 9f14bdc Compare May 9, 2024 04:24
@mahf708
Copy link
Contributor

mahf708 commented May 9, 2024

@bishtgautam, let's take this chance to add a testmod for this specific case?

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024 •
edited
Loading

PR Preview Action v1.4.7
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6405/
on branch gh-pages at 2024-05-11 21:25 UTC

@bishtgautam
Copy link
Contributor Author

@mahf708 I found a bit of incorrect computation, but it doesn't cause an error because the computation is done correctly later in the code. I'm going to remove the incorrect computation in this PR itself. I will also update the water budget to be consistent with the C budget changes. Then, I will follow your suggestion and add a new test.

@bishtgautam bishtgautam changed the title Fixes the annual C budget in ELM [WIP] Fixes the annual C budget in ELM May 9, 2024
@bishtgautam bishtgautam mentioned this pull request May 9, 2024
Closed
bishtgautam added 3 commits May 8, 2024 21:37
@bishtgautam
Deletes the incorrect code for C-budget
5cdf5cc 
The code was incorrectly setting the beginning of month C values
to be equal to the C values at the end of ELM time step.
@bishtgautam
Removes a similarly incorrect code in water budget
436b2bb
@bishtgautam
Similarly update water states at first ELM step
5cbba49
@mahf708
Copy link
Contributor

mahf708 commented May 9, 2024

Sounds good, let me know if I can be of any help :)

@rljacob rljacob added the Land label May 9, 2024
@bishtgautam
Adds a 5-day SMS test
7d60be6 
The test starts on 0001-12-28 and runs till (0002-01-01). Thus, it
computes monthly, annual, and all_time budgets.
@bishtgautam bishtgautam changed the title [WIP] Fixes the annual C budget in ELM Fixes the annual C budget in ELM May 9, 2024
@bishtgautam bishtgautam added the BFB PR leaves answers BFB label May 9, 2024
mahf708
mahf708 reviewed May 9, 2024
View reviewed changes
cime_config/tests.py Outdated Show resolved Hide resolved
mahf708
mahf708 approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@mahf708 mahf708 left a comment

Choose a reason for hiding this comment

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

This looks good to me and it solves the issues it was designed to address. I think we will add a restart-style test (e.g., ERS) instead of SMS here, and we can later have this in the standard test suite. Thanks @bishtgautam for the quick fix!

@bishtgautam bishtgautam requested a review from mahf708 May 11, 2024 20:58
peterdschwartz added a commit that referenced this pull request May 23, 2024
@peterdschwartz
Merge branch 'bishtgautam/lnd/fixes-cnp' into next (PR #6405)
d2774ec 
The C-states for all budgets (daily/monthly/annual/instantaneous/all time)
are updated on the first ELM step.
Adds a 5-day SMS test that starts on 0001-12-28 and runs till 0002-01-01.
Fixes #6337
[non-BFB] for tests greater than a year with C budgets.
@peterdschwartz
Copy link
Contributor

Merged to next.

Expecting these DIFFs in the TCS and TWS variables

  ERS.r05_r05.ICNPRDCTCBC.pm-cpu_intel.elm-cbudget (Overall: DIFF) details:
  SMS_Ly2_P1x1.1x1_smallvilleIA.IELMCNCROP.pm-cpu_intel.elm-force_netcdf_pio (Overall: DIFF) details:
  SMS_Ly2_P1x1_D.1x1_smallvilleIA.IELMCNCROP.pm-cpu_intel.elm-lulcc_sville (Overall: DIFF) details:

@peterdschwartz peterdschwartz removed the BFB PR leaves answers BFB label May 23, 2024
@peterdschwartz peterdschwartz merged commit c071a31 into master May 23, 2024
21 checks passed
@peterdschwartz peterdschwartz deleted the bishtgautam/lnd/fixes-cnp branch May 23, 2024 18:38
@peterdschwartz
Copy link
Contributor

Merged to master. Still need to wait for pm-cpu_gnu to report

@rljacob
Copy link
Member

rljacob commented Sep 30, 2024

@bishtgautam I'm not clear on how to explain the bug and the fix. Annual carbon budgets were zeroed if a run started in the middle of a year? And now they are not?

@bishtgautam
Copy link
Contributor Author

@rljacob How about the following description?

The initialization of the carbon budget in ELM is fixed to allow for the case when the model starts mid-month.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahf708 mahf708 mahf708 approved these changes

Assignees

@peterdschwartz peterdschwartz

Labels
Land
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CNP Budget failure for a non-Jan starting run.
4 participants
@bishtgautam @mahf708 @peterdschwartz @rljacob