-
Notifications
You must be signed in to change notification settings - Fork 312
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
Implementation of irrigation stream file #2102
base: master
Are you sure you want to change the base?
Conversation
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 for doing the work to convert your previous PR into one that works with stream files! It enables much more flexibility in future experiments.
I have a few questions, comments, and suggestions that I've added to the code in this PR. In addition:
- Does this close issue Implementation of flood irrigation and paddy irrigation as well as the irrigation method dataset #1696? I think it will, if you provide a stream file to use to satisfy the second item there. (I'll need that stream file for testing, regardless.)
- For clarity, I think we should rename these arrays:
irrig_method_patch
should be renamed toirrig_method_stream_patch
irrig_method
should be renamed toirrig_method_surf_gridcell
Finally, there are various minor issues (like whitespace, typos, etc.) that I didn't bother noting.
I've "requested changes," but I don't think it'd be right for me to ask you to do these yourself as—although they're simple—I know how busy the last part of your PhD is. The best way to handle this moving forward, I think, would be for you to give me collaborator access to your repository. I would then be able to work on your branch directly. If you're not comfortable with that, I could instead submit pull requests—whichever you prefer.
src/biogeophys/SurfaceWaterMod.F90
Outdated
if (h2osfc(c) > 100) then | ||
qflx_h2osfc_surf(c) = (h2osfc(c) - 100) / dtime |
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.
- Instead of hard-coding 100, have
surface_water_ponding_column
be a value in mm and use it here.
src/biogeophys/SurfaceWaterMod.F90
Outdated
if (irrigation_inst%surface_water_ponding_column(c) > 0) then | ||
frac_infclust=0.0_r8 |
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.
-
frac_infclust
does not seem to be used if surface water ponding is on, suggesting that this change can be reverted.
character(len=CL) :: irrig_tintalgo = 'nearest' ! Time interpolation alogrithm | ||
character(len=CL) :: stream_mapalgo = 'nn' | ||
real(r8) :: stream_dtlimit = 15._r8 | ||
character(len=CL) :: stream_taxmode = 'cycle' |
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.
cycle
makes sense if, for instance, you're providing irrigation amounts for every day in just one year. extend
would make more sense in the context of Yao et al. (2022), where you're providing irrigation techniques for what may be a limited number of years.
- Make it so irrigation stream file's
stream_taxmode
can be set as namelist parameter. - Ensure that default value corresponds to what should happen for default streams file.
<stream_year_last_irrig >1850</stream_year_last_irrig> | ||
<model_year_align_irrig >1850</model_year_align_irrig> | ||
|
||
<stream_fldfilename_irrig hgrid="0.9x1.25">lnd/clm2/prescribed_data/LFMIP-pdLC-SST.H2OSOI.0.9x1.25.20levsoi.natveg.1980-2014.MONS_climo.c190716.nc</stream_fldfilename_irrig> |
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.
-
stream_fldfilename_irrig
default should be removed, as it points to a soil moisture file. Alternatively, you could provide a file to use. (I'll need that file anyway for testing, but we may not want to provide that as a default dataset.)
<stream_fldfilename_irrig hgrid="0.9x1.25">lnd/clm2/prescribed_data/LFMIP-pdLC-SST.H2OSOI.0.9x1.25.20levsoi.natveg.1980-2014.MONS_climo.c190716.nc</stream_fldfilename_irrig> | ||
|
||
<irrig_mapalgo>nn</irrig_mapalgo> | ||
<irrig_tintalgo>nearest</irrig_tintalgo> |
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.
-
irrig_tintalgo
andirrig_mapalgo
must always benearest
/nn
for categorical variables likeirrig_method_patch
, for which interpolation isn't meaningful. Options to fix: - Remove these as something that can be changed
- Silently use
nearest
/nn
to affect categorical variables - Fail if something other than
nearest
/nn
is used but categorical variables are read from the streams file
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.
From conversation with @swensosc: For now, I will leave irrig_tintalgo
and irrig_mapalgo
as namelist parameters, but only allow nearest
/nn
. This will provide the framework for eventually adding other options if users request them.
|
||
<stream_fldfilename_irrig hgrid="0.9x1.25">lnd/clm2/prescribed_data/LFMIP-pdLC-SST.H2OSOI.0.9x1.25.20levsoi.natveg.1980-2014.MONS_climo.c190716.nc</stream_fldfilename_irrig> | ||
|
||
<irrig_mapalgo>nn</irrig_mapalgo> |
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.
-
irrig_mapalgo
appears in XML files but not Fortran or CLMBuildNamelist.pm. Add code to handle it.
case ('irrig_longitude') | ||
allocate(irrig_lon(bounds%begg:bounds%endg,num_irrig_cft)) | ||
do g = bounds%begg, bounds%endg | ||
ig = g_to_ig(g) | ||
irrig_lon(g,:) = dataptr2d(:,ig) | ||
enddo | ||
case ('irrig_latitude') | ||
allocate(irrig_lat(bounds%begg:bounds%endg,num_irrig_cft)) | ||
do g = bounds%begg, bounds%endg | ||
ig = g_to_ig(g) | ||
irrig_lat(g,:) = dataptr2d(:,ig) | ||
enddo |
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.
- Make it so that
irrig_longitude
andirrig_latitude
, intended to allow running a global grid with a regional irrigation stream file, are actually used.
@@ -202,6 +210,7 @@ module IrrigationMod | |||
procedure, private :: InitAllocate => IrrigationInitAllocate | |||
procedure, private :: InitHistory => IrrigationInitHistory | |||
procedure, private :: InitCold => IrrigationInitCold | |||
procedure, private :: UpdateTargetSMP => IrrigationUpdateTargetSMP |
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.
-
IrrigationUpdateTargetSMP
doesn't need theUpdateTargetSMP
alias.
src/biogeophys/IrrigationMod.F90
Outdated
integer :: dtime ! land model time step (sec) | ||
integer :: irrig_nsteps_per_day ! number of time steps per day in which we irrigate | ||
integer :: dtime ! land model time step (sec) | ||
integer, pointer :: irrig_nsteps_per_day (:) ! number of time steps per day in which we irrigate [col] |
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.
-
irrig_nsteps_per_day
needs_column
suffix.
@@ -235,6 +244,9 @@ module IrrigationMod | |||
integer, parameter, public :: irrig_method_drip = 1 | |||
! Sprinkler is applied directly to canopy | |||
integer, parameter, public :: irrig_method_sprinkler = 2 | |||
! Flood is applied to soil surface | |||
!(currently, the only difference between drip and flood is the target soil water, which can be implemented in Irrigation stream files. We leave it here in case future changes) |
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.
- Could you elaborate on this?
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.
Basically, we may not need a separate irrig_method_flood
, because that behavior will be entirely controlled by the stream file values. The difference between drip and sprinkler here is just whether water is applied above or below canopy.
- Get rid of
irrig_method_flood
.
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.
Yes it is true but I am still wondering in the future, the drip irrigation shall be re-designed. In the current version of the code, the drip irrigation would also cause many runoff, which is not very realistic. Just some thoughts: like applying water into the soil directly?
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.
Good point—there might eventually be a true "drip" irrigation that's distinct from a below-canopy sprinkler. @swensosc, what do you think?
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.
Sorry, I see now that your comment about crop_fsat_equals_zero
was intended for this discussion. I'm not sure I understand what you're saying, though.
src/biogeophys/IrrigationMod.F90
Outdated
! if using irrigation streams, do not set these two variables | ||
! TODO: determine volr limited rate based on prescribed input rates rather than soil moisture based irrigation rates | ||
! if (.not. use_irrigation_streams) then | ||
! Convert units from mm to mm/sec | ||
this%sfc_irrig_rate_patch(p) = deficit_volr_limited(c) / & | ||
(this%dtime*this%irrig_nsteps_per_day(c)) | ||
this%irrig_rate_demand_patch(p) = deficit(c) / & | ||
(this%dtime*this%irrig_nsteps_per_day(c)) | ||
! else | ||
! this%sfc_irrig_rate_patch(p) = 0._r8 | ||
! this%irrig_rate_demand_patch(p) = 0._r8 | ||
! endif |
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.
-
Is theIt seems like it might have been intended to ensure that the fallback settings are used where these variables aren't on the stream file (or there is no stream file). Check that this is handled correctly.if (.not. use_irrigation_streams)
andelse
stuff meant to be commented out? - Ensure that behavior is correct when prescribing irrigation rate but also limiting based on available river water.
logical :: irrig_ignore_data_if_missing ! If should ignore overridding a point with soil moisture data | ||
! from the streams file, if the streams file shows that point | ||
! as missing (namelist item) |
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.
irrig_ignore_data_if_missing
is currently unused.
- Look at soil moisture streams to see how they used
irrig_ignore_data_if_missing
. Decide if it should be deleted or used.
Some additional things upon further inspection/testing/discussion with @swensosc (some of which solve issues that predate this PR):
|
All unit test results seem to be zero. This happens with soil_water_retention_curve uninitialized in calculateIrrigation(), as in this commit, as well as with it initialized. Making it pointer instead of allocatable makes no difference in either case.
Sorry Sam I may close it accidently. |
I think the "crop_fsat_equals_zero" reduces runoff for crop, so that only
infiltration excess runoff occurs when it is active.
…On Sun, Aug 20, 2023 at 3:15 AM YiYaoVUB ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/biogeophys/IrrigationMod.F90
<#2102 (comment)>:
> @@ -235,6 +244,9 @@ module IrrigationMod
integer, parameter, public :: irrig_method_drip = 1
! Sprinkler is applied directly to canopy
integer, parameter, public :: irrig_method_sprinkler = 2
+ ! Flood is applied to soil surface
+ !(currently, the only difference between drip and flood is the target soil water, which can be implemented in Irrigation stream files. We leave it here in case future changes)
Yes it is true but I am still wondering in the future, the drip irrigation
shall be re-designed. In the current version of the code, the drip
irrigation would also cause many runoff, which is not very realistic. Just
some thoughts: like applying water into the soil directly?
—
Reply to this email directly, view it on GitHub
<#2102 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGRN57H4ERFB3RYFRECRSGDXWHITRANCNFSM6AAAAAA3PXECCU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
my comment seems to have been placed at the bottom of the discussion, but
it was:
I think the "crop_fsat_equals_zero" reduces runoff for crop, so that only
infiltration excess runoff occurs when it is active.
So basically, I think our treatment of runoff can be handled separately
from the treatment of irrigation method.
…On Tue, Aug 22, 2023 at 9:53 AM Sam Rabin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/biogeophys/IrrigationMod.F90
<#2102 (comment)>:
> @@ -235,6 +244,9 @@ module IrrigationMod
integer, parameter, public :: irrig_method_drip = 1
! Sprinkler is applied directly to canopy
integer, parameter, public :: irrig_method_sprinkler = 2
+ ! Flood is applied to soil surface
+ !(currently, the only difference between drip and flood is the target soil water, which can be implemented in Irrigation stream files. We leave it here in case future changes)
Good point—there might eventually be a true "drip" irrigation that's
distinct from a below-canopy sprinkler. @swensosc
<https://github.com/swensosc>, what do you think?
—
Reply to this email directly, view it on GitHub
<#2102 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGRN57CWECHIG5I23IEFWYTXWTIW3ANCNFSM6AAAAAA3PXECCU>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There is more work to be done here. So I'm changing it to a draft. It's still important that just helps us to see what is ready to go now and what is a work in progress. |
Description of changes
In this pull request, we added the irrigation stream file where you can customize the irrigation-related parameters.
Before: Most of the irrigation-related parameters apply to all crops and all gridcells. You just need to set them in user namelist files
After: Now you can give different values to irrigation-related files for different crops and different gridcells.
Specific notes
Please note that this pull request is replacing the pull request #2026
Contributors other than yourself, if any:
@swensosc @samsrabin
CTSM Issues Fixed (include github issue #):
#2026
It does not only fix the issue but also grants users more flexibility. In next steps, a default irrigation stream files or its generation tools may be given
Are answers expected to change (and if so in what way)?
Any User Interface Changes (namelist or namelist defaults changes)?
yes. in the namelist we can add if use_irrigation_stream is true or wrong
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 intel/gnu and izumi for intel/gnu/nag/pgi is the standard for tags on master)
NOTE: Be sure to check your coding style against the standard
(https://github.com/ESCOMP/ctsm/wiki/CTSM-coding-guidelines) and review
the list of common problems to watch out for
(https://github.com/ESCOMP/CTSM/wiki/List-of-common-problems).