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

Make bgc balance_check_tolerance a nml variable and increase the default #6651

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

wlin7
Copy link
Contributor

@wlin7 wlin7 commented Sep 27, 2024

This enables namelist control for the balance_check_tolerance.
The hardwired default of balance_check_tolerance is increased from
1e-8 to 1e-7 for C, N and P based on testing results from investigating a runtime
failure due to grid and column nbalance errors exceeding specified
threshold.

[BFB] for tests never having nbalance issue
[NML] no NML diff for tests as default is not set via nml.


Tested for the v3.LR.historical_0201 case that failed in 1996-02 with the original threshold, confirm the effectiveness of the nml control and the new default value for overcoming the nbalance issue.

Previous testings showed the error can reach more than 5e-8 but sufficiently distant from the new default of 1e-7

@wlin7 wlin7 added Land BFB PR leaves answers BFB NML labels Sep 27, 2024
@wlin7 wlin7 added this to the v3.0.1 milestone Sep 27, 2024
Copy link

github-actions bot commented Sep 27, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6651/
on branch gh-pages at 2024-09-30 19:56 UTC

Copy link
Contributor

@bishtgautam bishtgautam left a comment

Choose a reason for hiding this comment

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

How about changing balance_check_tolerance to bgc_balance_check_tolerance?

@wlin7
Copy link
Contributor Author

wlin7 commented Sep 27, 2024

How about changing balance_check_tolerance to bgc_balance_check_tolerance?

Good idea to add a distinction at the component model level. I opt to rename the namelist variable as suggested while keeping the name in the original code EcosystemBalanceCheckMod.F90 unchanged. An inline comment is added there as a reference.

@peterdschwartz
Copy link
Contributor

This change is missing from some of the balance check routines. For example, GridCBalanceCheck, ColPBalanceCheck, and ColCBalanceCheck that i saw.

@wlin7
Copy link
Contributor Author

wlin7 commented Sep 30, 2024

This change is missing from some of the balance check routines. For example, GridCBalanceCheck, ColPBalanceCheck, and ColCBalanceCheck that i saw.

Indeed @peterdschwartz . The original hardwired tolerance uses the same 1e-8 for C, N, and P balance errors. The changes here were mainly targeting the nbalance issue encountered during v3.LR simulations. Do you feel safe to also apply the increased tolerance to C and P balance check? It may sound good to do so as the nml variable was not specifically named for 'N' balance check.

@rljacob
Copy link
Member

rljacob commented Sep 30, 2024

@bishtgautam or @peterdschwartz do you want further changes to this PR? Its the last one for v3.0.1.

@peterdschwartz
Copy link
Contributor

I think that C,N,P should have the same threshold for balance errors as the model treats them symmetrically whenever it can. If applying the change consistently creates more issues, then that could be indicating a real error rather than accumulation of floating point errors.

@bishtgautam
Copy link
Contributor

@peterdschwartz Are you suggesting the following additional changes?

  • if (abs(col_errpb(c)) > 1e-8_r8) then to if (abs(col_errpb(c)) > bgc_balance_check_tolerance) then
  • if (abs(col_errcb(c)) > 1e-8_r8) then to if (abs(col_errcb(c)) > bgc_balance_check_tolerance) then

If yes, I agree with you that those should be changed too.

@bishtgautam
Copy link
Contributor

@wlin7 , What do you think?

@wlin7
Copy link
Contributor Author

wlin7 commented Sep 30, 2024

Sounds good to me to apply the threshold to all - C.N.P. It is done. Thank you @peterdschwartz @bishtgautam .
Since the larger error so far only occur to N balance at a particular grid cell (17.75S, 52.25W in the r05 mesh), if later having a better understanding of the budget calculation at this grid, the default threshold can be reset via nml without code change.

@rljacob
Copy link
Member

rljacob commented Sep 30, 2024

@wlin7 Did you verify this doesn't change answers?

@wlin7
Copy link
Contributor Author

wlin7 commented Sep 30, 2024

@wlin7 Did you verify this doesn't change answers?

@rljacob , the changes will not change answer for successful runs. All those balance checks would end the runs if the error threshold is exceeded. No C and P balance errors have been reported in simulation campaigns or regular tests. In theory, if runs are not aborted when threshold is exceeded, varying threshold could lead to NBFB.

@rljacob
Copy link
Member

rljacob commented Sep 30, 2024

@bishtgautam can you merge this to next today?

rljacob added a commit that referenced this pull request Oct 1, 2024
This enables namelist control for the balance_check_tolerance.
The hardwired default of balance_check_tolerance is increased from
1e-8 to 1e-7 for C, N and P based on testing results from investigating a runtime
failure due to grid and column nbalance errors exceeding specified
threshold.

[BFB] for tests never having nbalance issue
[NML] no NML diff for tests as default is not set via nml.
@bishtgautam bishtgautam merged commit b663752 into master Oct 1, 2024
9 checks passed
@bishtgautam bishtgautam deleted the wlin/lnd/nml_nbalance_err_tol branch October 1, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BFB PR leaves answers BFB Land NML
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants