-
Notifications
You must be signed in to change notification settings - Fork 446
Negative runoff quick-fix #7809
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
base: master
Are you sure you want to change the base?
Conversation
|
@hydrotian Please can you provide a location of the coupled simulation with these changes for us to explore? Also can you provide diagnostics for this simulation? Finally, can you confirm that these changes pass SMS, PET, PEM and ERS tests in a B-case? |
|
@proteanplanet I don't have a coupled simulations done with this PR yet but I plan to submit one following my previous Bluetip simulation. This PR passed the |
|
Those test results don't have any PET or PEM tests. Try PET.ne4pg2_ne4pg2.I1850CNPRDCTCBCTOP and PEM.ne4pg2_ne4pg2.I1850CNPRDCTCBCTOP |
|
@rljacob The It is strange as I did not modify the land model in this PR. Any ideas? Should I try it on Chrysalis instead? |
|
Yes try chrysalis. There may not be a good pelayout for that case on compy. |
| integer, allocatable :: outlet_gindices_local(:) ! Local array of global indices of outlets on this task | ||
| real(r8), allocatable :: outlet_discharges_local(:) ! Local array of discharges for these outlets | ||
| integer :: local_outlet_count | ||
| integer, allocatable :: all_outlet_gindices(:) ! Gathered on master |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A number of these variables look unused.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are removed. Thanks.
|
|
||
| ! Reproducible sum for negative qgwl | ||
| neg_local(1,1) = local_negative_qgwl_sum | ||
| call shr_reprosum_calc(neg_local, neg_global, 1, 1, 1, & |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could combine the two calls to shr_reprosum_calc into one call because it looks like the two fields are independent of each other. That would be more efficient than two calls.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. The two calls are now combined.
|
I ran a PEM.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel test and it failed the comparison between the two runs. The PEM_Ln9.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel test that's in e3sm_integration passes, but since it's only running 9 steps mosart only runs once in that test |
My |
|
Thanks @hydrotian -- I checked and both runs for my PEM test completed fine, just had different results. I'm running a similar PET test right now |
|
@jonbob Thanks. Could you share the |
|
Sure, but after five days it ends up with 351 out of 507 fields different. It's at: |
|
OK, the similar PET test (PET.ne30pg2_r05_IcoswISC30E3r5.WCYCL1850.chrysalis_intel) passed |
|
No insights from the PEM test -- we would have to do one where we tried to catch the first field that gets different answers. @proteanplanet noticed that you have a routine for sort_outlets_by_discharge_desc but we couldn't see it getting called? |
|
Yes. That was from an earlier commit on this branch. I can clean it up. |
|
To get a better idea of when it diffs, change the river coupling frequency to match the other models. That might allow you to go back to a 9 nstep test. Also change the coupler history output to be every timestep. |
|
@hydrotian -- I set redirect_negative_qgwl = .false. in your branch and the PEM test passes |
This reverts commit df6114f.
|
The PEM test has passed now. Both @jonbob and I confirmed that on our separate tests. |
A quick-fix to eliminate the negative runoff sent from ROF to OCN. Activated by setting
redirect_negative_qgwl = .true.inuser_nl_mosart. Two scenarios considered:Scenario A (net_global_qgwl ≥ 0):
Scenario B (net_global_qgwl < 0):