Groundwater irrigation #523
Conversation
|
@swensosc Regarding our in-person conversation about why the volr-based limitation is in calcIrrigationNeeded yet I'm suggesting moving the groundwater-related logic into ApplyIrrigation: I found this comment in the ChangeLog entry for
The key difference is that, for volr, we don't have an easy way to determine if this is a time step when we have an updated volr value, or if we still have volr from a few time steps ago (since ROF isn't coupled in every time step). So in that case, having the possibility that we'd exceed available volr seemed like the lesser of evils. In the case at hand, however, we can rely on having an up-to-date view of available groundwater, so the argument against having the volr limitation in ApplyIrrigation doesn't apply, and that seems like a more robust place to do this limitation. That said, I also see the argument that it would be nice to have both volr-based and groundwater-based irrigation in the same place, if we're willing to live with one of them being in a less-than-ideal place. I'm happy to discuss this more if you want. |
…to new routine CalcAvailableUnconfinedAquifer
Use filter loops in ApplyIrrigation, this requires initializing some variables.
|
@swensosc thanks a lot for making the fixes to histFileMod here. @ekluzek I feel like you're better qualified to review the changes to histFileMod, since I think you've more recently worked with this index stuff. So I added you as a reviewer, but can you please just review the changes to histFileMod and determine if this fully fixes #81 ? (Of course, feel free to review other parts of this PR, but I feel better able to review the other pieces myself.) |
There was a problem hiding this comment.
@swensosc thanks a lot for making all of these changes, and sorry once again that it took me so long to review your changes. I really appreciate your careful addressing of my earlier comments. I have some remaining comments; most of them are cosmetic, but there are some possibly substantive comments (i.e., relating to the science/implementation) related to your changes in SoilHydrologyMod, and another possibly substantive comment in CanopyHydrology.
Resolved Conflicts: src/biogeophys/IrrigationMod.F90
|
@swensosc I have merged this up to the latest master, and tested the build (but not a run). |
…o groundwater_irrigation merge to trunk
This reverts commit 3a43fbd. I've decided that this adds complexity without providing huge benefit (it gives more of a false confidence than any huge benefit). I'm planning to go with a strategy of having as much code as possible covered by ERP system tests, and not trying to rely on unit tests to test multi-point behavior.
…t tests" This reverts commit e6942dc. Since I'm not turning all of my single-point tests into multi-point tests after all, this isn't necessary.
This is used to set the irrigation method if it is not specified on the surface dataset. This is particularly useful for testing, until we can rely on having this on all surface datasets.
use_groundwater_irrigation only makes sense if limit_irrigation_if_rof_enabled is set (if limit_irrigation_if_rof_enabled is .false., then groundwater extraction will never be invoked).
|
@swensosc - I'm getting differences in I haven't taken the time to understand this fully; can you confirm that I should make the changes in parentheses above or some different changes to fix this? |
…o groundwater_irrigation
|
I think you're correct. I made the changes and one other place where
'namel' should have been used.
…On Thu, Nov 15, 2018 at 5:22 AM Bill Sacks ***@***.***> wrote:
@swensosc <https://github.com/swensosc> - I'm getting differences in
cols1d_gi, pfts1d_gi and pfts1d_li when I change processor / thread
count. At a glance, it looks like this may be due to the fact that you use
clmlevel=namel for the GetGlobalIndex call for cols1d_gi (should that be
clmlevel=nameg?), and clmlevel=namec for the calls for pfts1d_gi and
pfts1d_li (should those be clmlevel=nameg and clmlevel=namel?).
I haven't taken the time to understand this fully; can you confirm that I
should make the changes in parentheses above or some different changes to
fix this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#523 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AaLe_DiQMhsng8gDD-RG0XxiL7d6JtPgks5uvVx0gaJpZM4W8-hh>
.
|
Point is to reduced complexity in the main driver
Avoid doing some unnecessary work if various conditionals are false
Without this, in runs with create_crop_landunit false, we run into problems trying to access irrig_method for the irrigated generic crop, which falls outside the bounds of clm_varsur's irrig_method array: at /gpfs/fs1/work/sacks/ctsm_code/current_branch1/src/biogeophys/IrrigationMod.F90:715 Fortran runtime error: Index '16' of dimension 2 of array 'irrig_method' below lower bound of 17 I'm not sure we want this check long-term, but we need it at least until the code for irrig_method is generalized to work with create_crop_landunit false.
This is an improvement over the previous commit in that (1) it is more general, in case we change how clm_varsur's irrig_method is allocated, and (2) it works for the unit tests.
|
There are baseline differences in select fields in a number of tests. Here is an email I just sent to @swensosc about this: I have been looking more into the diffs in WA that we were discussing yesterday. Here are a few findings: (1) All of the differences are indeed in tests with cold start initial conditions, except for one test that shows diffs that is a CLM50 test that points to CLM45 initial conditions. (2) Spot-checking one of these tests: On master, WA was exactly 5000 everywhere at the end of the run. On the branch, WA was roughly 4000 everywhere (in some grid cells it is 4000, in others it is about 3999.9990). So this is a big change! I think this arises because wa_col is initialized to 4000; on master, it was then changed to 5000 in the first time step, but this no longer happens on the branch. Note that the aquifer_water_baseline parameter is 5000, and there are some notes in TotalWaterAndHeatMod about assuming that wa = aquifer_water_baseline in CLM50... so if we are changing wa to typically be ~ 4000, then we should give some thought to this TotalWaterAndHeat code. (3) While I'm not 100% confident of this, I've managed to convince myself that all of the differences we're seeing can be traced back to this change in wa from 5000 to 4000: this leads to roundoff-level differences in both the pre and post-landuse state, and since the dynbal adjustment fluxes are the small differences between large values, the flux differences end up looking greater than roundoff (see also #570). Yesterday, we talked about my doing some extra baseline comparisons where I back out Jinyun's changes in SoilWaterMovement in both the baselines and the branch. But I don't think this is worth doing until (2) is resolved, because I'm pretty sure we're still going to see differences from baseline as long as wa has been changed by this much. We can talk more when you're back the week after Thanksgiving. One thought you could consider at that point – if this is going to take more time than you are able to devote to it before AGU – is that I could move ahead with the groundwater_irrigation branch but with the BalanceCheckMod change backed out, and we could open an issue to fix this in a follow-on tag. |
In the groundwater_irrigation branch (ESCOMP#523), @swensosc has stopped resetting wa_col each time step if use_aquifer_layer is false. However, this leads to having a substantially different value of wa_col when use_aquifer_layer is false: previously, it was reset to aquifer_water_baseline each time step, but with the changes from @swensosc, it stays close to its initial values, which have been 4000 in most places. This commit changes the initial values to match the value it was being reset to, so it simply starts at aquifer_water_baseline - so the every-time-step-resetting to aquifer_water_baseline can be removed without massively changing the value of wa_col. In addition to changing the cold start initialization of wa_col, we are also changing the cold start initialization of zwt in this case where use_aquifer_layer is false: The old initialization of zwt wouldn't work as intended now that we have changed the initial value of wa_col; @swensosc suggested this new initialization method instead. This initialization to zi(c,nbedrock(c)) is correct if there are no saturated layers, and close enough for a decent cold start even if there are.
Rework cold start initialization of wa and zwt when use_aquifer_layer is false to reduce answer changes in upcoming groundwater_irrigation branch. In the groundwater_irrigation branch (#523), Sean Swenson has stopped resetting wa_col each time step if use_aquifer_layer is false. However, this leads to having a substantially different value of wa_col when use_aquifer_layer is false: previously, it was reset to aquifer_water_baseline each time step, but with Sean's changes, it stays close to its initial values, which have been 4000 in most places. This tag changes the initial values to match the value it was being reset to, so it simply starts at aquifer_water_baseline - so the every-time-step-resetting to aquifer_water_baseline can be removed without massively changing the value of wa_col. In addition to changing the cold start initialization of wa_col, we are also changing the cold start initialization of zwt in this case where use_aquifer_layer is false: The old initialization of zwt wouldn't work as intended now that we have changed the initial value of wa_col; Sean Swenson suggested this new initialization method instead. This initialization to zi(c,nbedrock(c)) is correct if there are no saturated layers, and close enough for a decent cold start even if there are.
Resolved Conflicts: src/main/surfrdMod.F90
Resolved Conflicts: src/biogeophys/BalanceCheckMod.F90 src/biogeophys/WaterDiagnosticBulkType.F90 src/biogeophys/WaterFluxType.F90 src/main/clm_driver.F90
The code for calculating per-layer water availability was inconsistent between CalcAvailableUnconfinedAquifer and WithdrawGroundwaterIrrigation. It seemed incorrect that, in WithdrawGroundwaterIrrigation, irrig_layer always had zi - zwt, instead of sometimes having zi(c,j) - zi(c,j-1) as in CalcAvailableUnconfinedAquifer. I think this problem is related to the more general bug in the calculation of rsub_top_layer that @swensosc initially fixed in ESCOMP#523 but I asked him to back out of that PR (deferring its fix to its own PR). Based on the fix he proposed there, it seems that the code in CalcAvailableUnconfinedAquifer is correct, and the code in WithdrawGroundwaterIrrigation is incorrect. This commit fixes the issue. Resolves ESCOMP#595
Sf fuels refactor + bole harvest litter logic fix/export frac
Description of changes
Add irrigation from groundwater sources (unconfined [from the soil column] and confined [from wa] aquifers). Add irrigation application method (sprinkler [above canopy] and drip [below canopy]). Both of these changes are activated via namelist. Add option to set saturated fraction to zero for crop columns.
Specific notes
Irrigation method also requires input data on surface data file, otherwise will default to previous behavior (drip).
Contributors other than yourself, if any:
CTSM Issues Fixed (include github issue #):
Fixes #81
Are answers expected to change (and if so in what way)?
maybe for changes related to irrigation method due to new variables used to
apply irrigation in CanopyHydrologyMod
Any User Interface Changes (namelist or namelist defaults changes)?
two new namelist variables: use_groundwater_irrigation and crop_fsat_equals_zero
the latter name might not be great and could be changed
Testing performed, if any:
(List what testing you did to show your changes worked as expected)
(This can be manual testing or running of the different test suites)
(Documentation on system testing is here: https://github.com/ESCOMP/ctsm/wiki/System-Testing-Guide)
(aux_clm on cheyenne for gnu/pgi and hobart for gnu/pgi/nag is the standard for tags on master)
performed 1 year simulation with new options activated
NOTE: Be sure to check your Coding style against the standard:
https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines