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

Chemistry output bug fix #6713

Merged
merged 1 commit into from
Nov 22, 2024
Merged

Chemistry output bug fix #6713

merged 1 commit into from
Nov 22, 2024

Conversation

jinboxie
Copy link
Contributor

@jinboxie jinboxie commented Oct 28, 2024

Due to a bug in the output of the chemistry reaction rate,
the r_lch4 and r_lco_h are output as 0 when no chemistry output
flags are set. The bug is fixed by modifying the if clause in the code.

Fixes #6711

[BFB]

Git info

  1. The output of reaction rate of r_lch4 and r_lco_h are not output due to a bug, fixed it in the code.

    modified: components/eam/src/chemistry/mozart/rate_diags.F90
    [BFB]

Copy link

github-actions bot commented Oct 28, 2024

PR Preview Action v1.4.8
🚀 Deployed preview to https://E3SM-Project.github.io/E3SM/pr-preview/pr-6713/
on branch gh-pages at 2024-11-04 23:35 UTC

@rljacob
Copy link
Member

rljacob commented Oct 28, 2024

Did you also want this on the maint-3.0 branch?

@jinboxie
Copy link
Contributor Author

Did you also want this on the maint-3.0 branch?

Yes. Rob @rljacob . The master as of 0305 (which should be that of maint-3.0) is also affected by this bug.

@rljacob
Copy link
Member

rljacob commented Oct 28, 2024

Then you will have to make a feature branch from before the maint-3.0 branch started and implement it there. This feature branch is after maint-3.0.

Will it change outputs for v3.0 production runs?

@jinboxie
Copy link
Contributor Author

Then you will have to make a feature branch from before the maint-3.0 branch started and implement it there. This feature branch is after maint-3.0.

Will it change outputs for v3.0 production runs?

Hi Rob @rljacob. I was making a simulation run that needed to validate these variables (r_lch4 and r_lco_h) to find that this code by predecessors seems to have this bug in the code.

These variables are reaction rates not produced by the production runs as we have checked here https://acme-climate.atlassian.net/wiki/spaces/CM/pages/4290281655/v3.LR.amip+simulations. The bug itself only makes the model fail to output these variables values (thus output 0), and should not affect the outputs for v3 production runs.

I'll make a separate maint-3.0 feature branch for it. Thank you for the suggestion :)

Best,
Jinbo

Copy link
Contributor

@hsiangheleellnl hsiangheleellnl left a comment

Choose a reason for hiding this comment

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

I don't understand the current propose for writing out r_lch4_2D and r_ch4_Lx. Before we need to turn on history_UCIgaschmbudget_2D_levels and hisUCIgasbudget_2D to write out the variables. Now, do we want to write out the variables matter what flag is on? If that is the case, why not just remove the "if" statement. Now, once the history_UCIgaschmbudget_2D_levels and hisUCIgasbudget_2D are on, the variables can not be written out.

Copy link
Contributor

@tangq tangq left a comment

Choose a reason for hiding this comment

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

@jinboxie , the PR looks good. Pls submit another PR to fix maint-3.0 as suggested by @rljacob

@hsiangheleellnl , the original output of rate_names should not be affected by the history_UCIgaschmbudget* flags, which is why this fix is needed.

Copy link
Contributor

@wlin7 wlin7 left a comment

Choose a reason for hiding this comment

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

The changes look good to have the output of related rates not affected by the history_UCIgaschmbudget flags.

@wlin7 wlin7 added the BFB PR leaves answers BFB label Oct 28, 2024
@jinboxie
Copy link
Contributor Author

jinboxie commented Oct 29, 2024

Then you will have to make a feature branch from before the maint-3.0 branch started and implement it there. This feature branch is after maint-3.0.

Will it change outputs for v3.0 production runs?

Hi Rob @rljacob . Please refer to #6716 for the fix for maint-3.0.

1. The output of reaction rate of r_lch4 and r_lco_h are
   not output due to a bug, fixed it in the code.

	modified:   components/eam/src/chemistry/mozart/rate_diags.F90
[BFB]
@jinboxie jinboxie force-pushed the jinboxie/atm/chemistry_output branch from 53d9dc2 to e8ecab3 Compare November 4, 2024 23:33
wlin7 added a commit that referenced this pull request Nov 21, 2024
Chemistry output bug fix

Due to a bug in the output of the chemistry reaction rate,
the r_lch4 and r_lco_h are output as 0 when no chemistry output
flags are set. The bug is fixed by modifying the if clause in the code.

Fixes #6711

[BFB]
@wlin7
Copy link
Contributor

wlin7 commented Nov 21, 2024

Merged to next.

@wlin7 wlin7 merged commit b6e16c6 into master Nov 22, 2024
9 checks passed
@wlin7 wlin7 deleted the jinboxie/atm/chemistry_output branch November 22, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

atmospheric chemistry reaction rate (r_lch4 and r_lco_h) output zero in e3smv3 master
5 participants