Correct MG2 rain number conservation limiter #1599
Conversation
|
I wanted to make sure that this was out there, because the issue that this PR fixes is definitely a bug, and I think it has slipped between the cracks. However, AFAIK no one has really evaluated the effect on ACME climate yet, so this shouldn't be merged yet. |
Fixes an incorrectly positioned parenthesis that caused the rain number
conservation code to reduce process rates by too much.
The rain number conservation code contained the following lines:
ratio = (nr(i,k)/deltat+nprc(i,k)*lcldm(i,k)/precip_frac(i,k))/ &
(-nsubr(i,k)+npracs(i,k)+nnuccr(i,k)+nnuccri(i,k)-nragg(i,k))*omsm
The values in the denominator (second line) represent concentrations in
the fraction of the grid cell where precipitation is present, while the
values `nr(i,k)` and `nprc(i,k)*lcldm(i,k)` in the numerator represent
mean concentrations over the entire grid cell.
In order to compare the two, the *entire* numerator should be divided by
the precipitation fraction, not just the `nprc` term. Thus this commit
moves the factor of `precip_frac(i,k)` outside of that set of
parentheses.
The precise impact of this change is not quite clear yet, but it is
potentially climate-changing, since fixing this bug will most likely
cause average drop size to increase, and therefore increase the rate of
precipitation. This has been demonstrated using an offline driver for
MG2, using a test case for which this bug is known to have a
particularly strong effect.
[CC]
75e312c to
2a65d9c
Compare
|
explanation of problematic line (to aid my own recollection if nobody else's): 'ratio' is the scaling factor applied to each of the process rates that reduces nr (rain number concentration) to prevent nr from going negative within a timestep. We want to choose ratio such that nr is exactly zero at the end of the step. We do this by setting ratio*(tendencies which decrease nr) = [nr/deltat + (tendencies which increase nr)*lcldm]/precip_frac. Note that tendencies which increase nr is multiplied by lcldm (cloud fraction) because those tendencies are in-cloud values and nr/deltat is a cell-average value. The whole right-hand side is divided by precip_frac because tendencies which decrease nr should also be in-precip rather than grid average values. The difference between the buggy and fixed version is that nr/deltat is not divided by precip frac (a number less than 1) in the buggy version, so its ratio will be too small. This will result in overly agressive throttling of processes reducing nr, so the buggy version should have higher nr than the fixed version. This line of code is only executed when tendencies try to make nr go negative within a timestep (i.e. when numerics is having problems), so hopefully it doesn't have a big impact on the model solution. I'm testing this now. |
|
Update: I ran F1850 (prescribed SST, constant 1850 conditions) for 5 yrs with and without this fix. Results are here: http://portal.nersc.gov/project/acme/coupled/beta/nrfix-on.F1850.ne30_ne30-nrfix-off.F1850.ne30_ne30_yrs2-5/ . It looks like this bug causes ~1.3 W/m2 change in TOA radiation balance. Based on the fact that this is definitely a bug and definitely has a climate impact, I think we need to get this on master... but we should do so carefully. In particular, we don't want to surprise people when the model behavior changes and we probably want to retune to compensate for this. |
|
Peter, I think the bug causes 0.88W/m2 difference in RESTOM, instead of 1.3W/m2. |
|
@polunma - yup, you're right. I read the table wrong. That mistake doesn't change the fact that this bug causes a substantial change to RESTOM, though. |
|
Agreed. |
|
@rljacob, @golaz , etc: @davidcbaderatllnl just told me he wants this bugfix on master ASAP. Because this will be significantly answer-changing, we should probably make a new beta tag for it. I'm not sure whether there are other things (e.g. in the ocean @jonbob ) that we want to incorporate into this tag... |
|
#1630 is good to go -- has passed threading tests on three different platforms. I'm just waiting for a review and can get it in immediately. But it's not critical for a new tag. We do hope to have some science changes very soon that will solve at least some of the mixing/noise issues. But that might not be until early next week. And again, that PR would be non-BFB. |
|
@PeterCaldwell , can you approve the PR? I will start merging shortly. |
|
Thanks @PeterCaldwell . @rljacob , since this is a climate changing PR, when is good time to merge it to next? |
|
Hi @rljacob , can this PR be merged as usual? Or does it need some arrangement given that it is a CC type of PR? |
|
It should go in as the only science-code or non-BFB change that day. |
|
Wait, the dashboard is a mess with fails from a previous PR. We need that to clear. |
|
Got it. Thanks Rob. |
|
@wlin7 go ahead and merge this to next today. |
Merge quantheory/atm/nr_precip_frac into next (PR #1599) Fixes an incorrectly positioned parenthesis that caused the rain number conservation code to reduce process rates by too much. The rain number conservation code contained the following lines: ratio = (nr(i,k)/deltat+nprc(i,k)*lcldm(i,k)/precip_frac(i,k))/ & (-nsubr(i,k)+npracs(i,k)+nnuccr(i,k)+nnuccri(i,k)-nragg(i,k))*omsm The values in the denominator (second line) represent concentrations in the fraction of the grid cell where precipitation is present, while the values nr(i,k) and nprc(i,k)*lcldm(i,k) in the numerator represent mean concentrations over the entire grid cell. In order to compare the two, the entire numerator should be divided by the precipitation fraction, not just the nprc term. Thus this commit moves the factor of precip_frac(i,k) outside of that set of parentheses. The precise impact of this change is not quite clear yet, but it is potentially climate-changing, since fixing this bug will most likely cause average drop size to increase, and therefore increase the rate of precipitation. This has been demonstrated using an offline driver for MG2, using a test case for which this bug is known to have a particularly strong effect. [CC]
|
merged to next. |
Update PGI compiler to 17.04 on hobart Update version of PGI compiler used on hobart Test suite: verified CECO.hobart_pgi would not build with current master, but does build and run with this branch Test baseline: N/A Test namelist changes: N/A Test status: I don't know if there are any compsets on hobart that build / run with pgi on master, but if so they probably won't have the same answers now Fixes #1599 User interface changes?: N/A Code review: one line changes are fun to review
|
Both machines doing testing on next have reported and results look as expected. @wlin7 please file tickets to bless failed tests and proceed to master. |
|
Tickets filed to bless failed (DIFF) tests for melvin and skybridge. Melvin reported 4 failed tests, all being DIFF. Request filed to bless all four. Skybridge reported 15 failed tests, 14 due to DIFF, but one, |
Merge quantheory/atm/nr_precip_frac (PR #1599) Fixes an incorrectly positioned parenthesis that caused the rain number conservation code to reduce process rates by too much. The rain number conservation code contained the following lines: ratio = (nr(i,k)/deltat+nprc(i,k)*lcldm(i,k)/precip_frac(i,k))/ & (-nsubr(i,k)+npracs(i,k)+nnuccr(i,k)+nnuccri(i,k)-nragg(i,k))*omsm The values in the denominator (second line) represent concentrations in the fraction of the grid cell where precipitation is present, while the values nr(i,k) and nprc(i,k)*lcldm(i,k) in the numerator represent mean concentrations over the entire grid cell. In order to compare the two, the entire numerator should be divided by the precipitation fraction, not just the nprc term. Thus this commit moves the factor of precip_frac(i,k) outside of that set of parentheses. The precise impact of this change is not quite clear yet, but it is potentially climate-changing, since fixing this bug will most likely cause average drop size to increase, and therefore increase the rate of precipitation. This has been demonstrated using an offline driver for MG2, using a test case for which this bug is known to have a particularly strong effect. [CC]
Thanks @wlin7 . This test can't be blessed as it is failing. SE team is aware of this so there is no need to file a ticket at this time for this test. |
|
Got it. Thanks @singhbalwinder . |
|
@rljacob , @singhbalwinder , |
|
Yes please do. |
|
Yes, a new ticket should be created for blessing the failing tests on Melvin and Chama. |
|
submitted. |
Fixes an incorrectly positioned parenthesis that caused the rain number
conservation code to reduce process rates by too much.
The rain number conservation code contained the following lines:
The values in the denominator (second line) represent concentrations in
the fraction of the grid cell where precipitation is present, while the
values
nr(i,k)andnprc(i,k)*lcldm(i,k)in the numerator representmean concentrations over the entire grid cell.
In order to compare the two, the entire numerator should be divided by
the precipitation fraction, not just the
nprcterm. Thus this commitmoves the factor of
precip_frac(i,k)outside of that set ofparentheses.
The precise impact of this change is not quite clear yet, but it is
potentially climate-changing, since fixing this bug will most likely
cause average drop size to increase, and therefore increase the rate of
precipitation. This has been demonstrated using an offline driver for
MG2, using a test case for which this bug is known to have a
particularly strong effect.
[CC]