Add check_flux_state_consistency to shoc interface #2418
Conversation
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: tcclevenger |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
3 similar comments
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Thanks for working on this @tcclevenger . I took a look at your code and the fortran snippet that Oksana added in issue #2372. I have big concerns with one of the lines here. It seems as if we are applying adjustment anytime the surface latent flux is less than zero (i.e. anytime cc < 0)? Is that true @oksanaguba? If so then I do not agree with this as cc < 0 (surface latent flux less than zero) is something that can and does happen quite often in nature and the model and SCREAM can handle this gracefully in 99.999999% of instances... However there are a few instances (primarily during spin-up) when the negative surface latent heat flux can exhaust the moisture in the lowest model level. It is only in these outlier cases where the fixer/adjust should be applied. Therefore I propose replacing line with a trigger something in the spirit in qneg4.F90 Where it checks to see if the bottom layer vapor will be exhausted or not. If it is, then carry on with Oksana's fixer. Else, if we do this whenever cflx < 0 if we will be overly applying this adjustment and I anticipate that simulation quality will be deteriorated. Apologies if I've misinterpreted the c++ code or oksana's code. |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
1 similar comment
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ oksanaguba ]! |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
Replying to Peter's @bogensch comment: Regarding how often QFLX<0 happens, while i don't think it is relevant to how we need to do the limiter, I think it happens often enough: check Figure 5 panel b for averaged QFLX, it seems there is enough of light green color in the plot (which indicates QFLX<0). thanks! |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
1 similar comment
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
|
||
| if (surf_evap(i) < 0) { |
There was a problem hiding this comment.
It seems as if we are applying adjustment anytime the surface latent flux is less than zero (i.e. anytime cc < 0)? Is that true @oksanaguba? If so then I do not agree with this as cc < 0 (surface latent flux less than zero) is something that can and does happen quite often in nature and the model and SCREAM can handle this gracefully in 99.999999% of instances... However there are a few instances (primarily during spin-up) when the negative surface latent heat flux can exhaust the moisture in the lowest model level. It is only in these outlier cases where the fixer/adjust should be applied. Therefore I propose replacing line
if (cc < 0.0_r8) then
with a trigger something in the spirit in qneg4.F90
excess(i) = qflx(i,1) - (qmin(1) - qbot(i,1))/(ztodt*gravit*srfrpdel(i))
if (excess(i) < 0._r8) then
Where it checks to see if the bottom layer vapor will be exhausted or not. If it is, then carry on with Oksana's fixer. Else, if we do this whenever cflx < 0 if we will be overly applying this adjustment and I anticipate that simulation quality will be deteriorated.
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However PR is now STALE, and must be retested. Set the AT: RETEST Label to force retest.... |
|
Thanks @oksanaguba but my issue is that as written we are calling this adjustment WAY more often than it is necessary and will likely have detrimental impacts on our simulation. This does not need to be triggered/called every instance the QFLX < 0 as the vast majority of the time no adjustment needs to be done. See my comment above. All I suggest is that the trigger be revised to be something similar to what qneg4 has (only call the adjustment when the application of the surface latent heat flux will exhaust/exceed all vapor at the surface). So just a one/two line change and the rest of the fixer I don't have a problem with. |
|
What I propose (using Oskana's code from issue #2372 as a basis) Note that only the first three lines within the ncol loop has changed. Everything else stays the same. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: tcclevenger |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Mappy # 4092 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Weaver # 4641 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Blake # 4688 (click to expand) |
|
@tcclevenger sorry for the confusion. I think placing the call in the SHOC interface before shoc_main is called is the best place. |
|
Status Flag 'Pull Request AutoTester' - Failure: Timed out waiting for job SCREAM_PullRequest_Autotester_Mappy to start: Total Wait = 1803
|
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: tcclevenger |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Mappy # 4094 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Weaver # 4643 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Blake # 4690 (click to expand) |
|
The FAIL's I see in the AT are DIFF's in I think this PR is ready to merge given a final round of reviews - @bogensch and @oksanaguba ? |
|
@AaronDonahue I'm currently running simulation on Summit to see what difference is made to the energy errors. I'll update here with what I find. |
|
@oksanaguba @bogensch Running |
|
@tcclevenger yes I believe the impact should be relatively small with this fixer. When I looked into this it appeared that (at ne30 anyway) this fixer was primarily only needed during the first few time time steps of the model (spin-up; but still a small percentage of columns), then after that there were only a very few isolated incidences where vapor was being exhausted by the surface fluxes. Thus, my experience with this is similar to your experience. |
|
Thanks, @bogensch. @bogensch and @oksanaguba, let me know if there are any other comments you have (and I'll need an approval). @AaronDonahue I'm going to re-run the AT since last was over 2 weeks ago and I can no longer check diffs. Then we can probably merge when that finishes. |
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: tcclevenger |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Mappy # 4134 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Weaver # 4674 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Blake # 4720 (click to expand) |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Using Repos:
Pull Request Author: tcclevenger |
|
@AaronDonahue Only diffs, no nans or massively different values (rel errs ~1e-8 to ~1e0). |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 4 Hrs. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: SCREAM_PullRequest_Autotester_Mappy
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Weaver
Jenkins Parameters
Build InformationTest Name: SCREAM_PullRequest_Autotester_Blake
Jenkins Parameters
Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Mappy # 4137 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Weaver # 4677 (click to expand)Console Output (last 100K lines) : SCREAM_PullRequest_Autotester_Blake # 4723 (click to expand) |
There was a problem hiding this comment.
Thanks for your efforts on this @tcclevenger. This looks good to me.
| // the moisture in the lowest model level. If so, apply fixer. | ||
| const auto condition = surf_evap(i) - (qmin - qv_i(last_pack_idx)[last_pack_entry])/(dt*gravit*rpdel); | ||
| if (condition < 0) { | ||
| const auto cc = abs(surf_evap(i)*dt*gravit); |
There was a problem hiding this comment.
For this case of a double, rather than pack, argument, I believe abs is C's int abs(int), not C++'s std::abs call through to, essentially, fabs. Demo:
$ cat abs.cpp
#include <iostream>
int main () { std::cout << abs(-3.14) << "\n"; }
$ g++ -std=c++14 abs.cpp; if [ $? == 0 ]; then ./a.out; fi
3
$ cat abs.cpp
#include <iostream>
int main () { std::cout << std::abs(-3.14) << "\n"; }
$ g++ -std=c++14 abs.cpp; if [ $? == 0 ]; then ./a.out; fi
3.14|
|
||
| surf_evap(i) = 0; | ||
| } | ||
| }); |
There was a problem hiding this comment.
i am confused about this code:
- i think it is being called in each macmic iteration, which means only surf_evap/macmic_loop_size is applied in shoc iteration. does the code operate with quantity surf_evap/macmic_loop_size or does it operate with surf_evap?
- the way i read the code, compared to the original version (before 'condition' line was introduced) is that nothing in the distribution of extra negative mass changed. i think Peter's objection was that if there is potentially negative mass in the bottom layer, then the excess would be redistributed above it. the way i read the code, right now not excess (value connected to 'condition'), but the whole surf_evap mass (value connected to 'cc') is redistributed, and not to the top of the very bottom layer, but to all layers (parallel_for loop).
- related to that, i think the algorithm that Peter B. wanted to see would leave surf_evap not with 0, but with the biggest by the abs value mass that is allowed in the bottom layer.
New
check_flux_state_consistency()from @oksanaguba suggestion #2372 (comment). Called directly beforeshoc_main().