Add river routing module #1143
Add river routing module #1143
Conversation
…t in GEOSgcm_GridComp/GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSland_GridComp/GEOSroute_GridComp
…ent (GEOS_RouteGridComp.F90)
Need to change RUN_ROUTE from 1 to 2 to make it zero-diff |
|
I have moved the river input to restart file and the model now reads the input from the restart. |
This comment was marked as resolved.
This comment was marked as resolved.
...GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/GEOSroute_GridComp/GEOS_RouteGridComp.F90
Show resolved
Hide resolved
|
Now you can have direct access to the Internal variables through "call MAPL_GetPointer(INTERNAL,...", you probably don't need the Reservior and Route object any more. More importantly, with the pointer to the internal variables, it will keep the changes of the internal variables @zyj8881357 |
…F90, GEOS_SurfaceGridComp.rc, preproc/routing_model/constant.f90preproc/routing_model/constant.f90)
There was a problem hiding this comment.
@zyj8881357 : I just reviewed some (but not all) of the changed files. I committed a minor cleanup (11fd3b3) and added a few in-line comments (see below). Also, can we rename:
preproc/routing_model/constant.f90 --> preproc/routing_model/routing_model_constants.f90 (avoiding the extremely generic "constant.f90" file name and also the conflict with the "routing_constant.f90" file name in "preproc/outlets/" )
PS: I have yet to review GEOS_RouteGridComp.F90, reservoir.F90, routing_model.F90, and pfaf_frac.F90. I'll take a look at these files as soon as I can and will submit a separate review.
|
|
||
| esma_add_library (${this} | ||
| SRCS ${srcs} | ||
| DEPENDENCIES MAPL GEOS_SurfaceShared GEOS_LandShared ESMF::ESMF NetCDF::NetCDF_Fortran GEOS_CatchCNShared |
There was a problem hiding this comment.
Do we actually need all these dependencies? Unless I missed something, the f90 files in this dir only have "use" statements for modules "catch_constants" and "routing_constant". The former is in "Land/Shared", and the latter is local. Can we drop the dependencies on "GEOS_SurfaceShared" and "GEOS_CatchCNShared"?
|
|
||
| esma_add_library (${this} | ||
| SRCS ${srcs} | ||
| DEPENDENCIES MAPL GEOS_SurfaceShared GEOS_LandShared ESMF::ESMF NetCDF::NetCDF_Fortran GEOS_CatchCNShared) |
There was a problem hiding this comment.
Do we need all these dependencies? (see similar comment on preproc/outlets/CMakeLists.txt)
| # Dimensions | ||
| nc_len = 291284 | ||
| nres = 7250 | ||
| nlake = 3917 |
There was a problem hiding this comment.
Some constants (eg., 291284) are defined in multiple python scripts. There should be one py file that holds all constants that are needed in more than one py script
…hanges for improved readability (GEOS_RouteGridComp.F90)
…similar statements (GEOS_RouteGridComp.F90)
…-> Q_riv_out); replaced if/else block with select case block (reservoir.F90)
…ted use of MAPL module (routing_model.F90)
gmao-rreichle
left a comment
gmao-rreichle
left a comment
There was a problem hiding this comment.
@zyj8881357 : I added a few commits and in-line comments (see below).
Here's a link to the aggregated changes from my commits (with white-space changes hidden):
https://github.com/GEOS-ESM/GEOSgcm_GridComp/compare/49790fa..f452878?w=1
The changes are focused on cleaning up the code and improving the documentation/comments (including units).
Please review my changes and let me know if anything looks wrong.
The in-line comments below include a couple of minor changes but also bigger things like changing which parameters should be in the bcs and routing restart files. It's probably best to address the changes that are zero-diff first, and verify that the revised branch produces zero-diff results. Once that is done, we can tackle the change in the routing (and reservoir) parameters, and verify that the resulting simulations produce scientifically identical results (should be within "roundoff")
Please let me know if you have any questions
cc: @weiyuan-jiang
| ! get time used for output and restart | ||
| !call ESMF_ClockGet(clock, currTime=CurrentTime, rc=status) | ||
| !call ESMF_TimeGet(CurrentTime, yy=YY, mm=MM, dd=DD, h=HH, m=MMM, s=SS, rc=status) | ||
| !write(yr_s, '(I4.4)')YY | ||
| !write(mon_s,'(I2.2)')MM | ||
| !write(day_s,'(I2.2)')DD | ||
| ! | ||
| !call check_balance(route,n_pfaf_local,nt_local,runoff_acc,WRIVER_ACT,WSTREAM_ACT,WTOT_BEFORE,RUNOFF_ACT,QINFLOW_LOCAL,QOUT_CAT,FirstTime,yr_s,mon_s) |
There was a problem hiding this comment.
I couldn't find the subroutine check_balance(). If it doesn't exist or is out of date, it's probably best to delete the commented-out code relating to check_balance() here, above, and below
| ! call reservoir module | ||
| call res%calc( QOUTFLOW_OUT, QRES_OUT, WRES, real(route_dt), _RC) | ||
| QOUT_CAT = QOUTFLOW_OUT | ||
| where(res%active_res==1) QOUT_CAT=QRES_OUT |
There was a problem hiding this comment.
The logic here seems a bit uneven.
First, why is QRES_OUT an intent(inout) in the calc() subroutine? I don't think the new reservoir outflow depends on the old reservoir outflow. Unless I'm missing something, QRES_OUT should be an intent(out) and should entirely be set from within calc().
Second, we should entirely bypass all reservoir calculations when RUN_ROUTE==1 (by adding an if condition around the reservoir calcs. Right now, we call the reservoir subroutine even when we're not doing reservoirs.
| !----Reservoir module constants---------- | ||
|
|
||
| integer, parameter :: nres = 7250 | ||
| integer, parameter :: nlake = 3917 |
There was a problem hiding this comment.
This number is also defined as nvo in Utils/Raster/preproc/routing_model/constant.f90. Does it have the same meaning, or is this just a coincidence? If it has the same meaning, we should limit the definition to one place
|
|
||
| case default | ||
|
|
||
| ! NEED TO ADD GRACEFUL EXIT WITH ERROR MESSAGE HERE: "ERROR: unknown reservoir type!" |
There was a problem hiding this comment.
need to add a graceful exit with error message here (but see comment above about this entire block)
|
|
||
| tmpfac = 1.0 / (this%wid_res(i) / 1.e3) | ||
|
|
||
| select case (this%type_res(i)) |
There was a problem hiding this comment.
I changed the if/else block here to "select case" so I could better understand what's done. It looks like the alpha values are based entirely on time-invariant constants, yet they are computed here from scratch within a do loop and a select case statement each time the reservoir calculations are executed. Also, the "wid_res" parameter seems to be used only for calculating the alpha parameter (and on top of that "wid_res" is simply the sqrt of "area_res" in preprocessing. To simplify this, it might be better to make "alpha" a routing model parameter (instead of "wid_res") and determine "alpha" in preprocessing.
| do xi = 1, nlon | ||
| do yi = 1, nlat | ||
| if (catchind(xi, yi) >= 1) then | ||
| ! The raster grid cell belongs to a catchment | ||
| id = catchind(xi, yi) ! Get the catchment id for the current cell | ||
| flag = 0 ! Reset flag to indicate whether a matching sub-area is found | ||
| ! If the catchment already has one or more sub-areas, check for a matching sub-area: | ||
| if (nsub(id) >= 1) then | ||
| do i = 1, nsub(id) | ||
| if (loni(xi) == xsub(i, id) .and. lati(yi) == ysub(i, id)) then | ||
| flag = 1 | ||
| exit ! Exit the inner loop since a matching sub-area has been found | ||
| endif | ||
| end do | ||
| endif | ||
| ! If no matching sub-area was found, create a new sub-area: | ||
| if (flag == 0) then | ||
| nsub(id) = nsub(id) + 1 | ||
| xsub(nsub(id), id) = loni(xi) | ||
| ysub(nsub(id), id) = lati(yi) | ||
| endif | ||
| endif | ||
| end do | ||
| end do |
There was a problem hiding this comment.
There is a nearly identical nested do loop in GEOSagcm_GridComp/GEOSphysics_GridComp/GEOSsurface_GridComp/Utils/Raster/preproc/routing_model/get_num_sub_catchment.f90
Not sure where this needs doing, but we shouldn't do the same stuff in both preproc and makebcs
| ! Open the catchment definition file for the EASE grid and read the total number of tiles (header) | ||
| open(77, file="clsm/catchment.def");read(77, *) ntot | ||
| ! Allocate arrays with size nt | ||
| allocate(tid(ntot), catid(ntot), lon_left(ntot), lon_right(ntot), lat_bottom(ntot), lat_top(ntot),latc(ntot),lonc(ntot)) | ||
| allocate(lati_tile(ntot),loni_tile(ntot),map_tile(nc_ease,nr_ease)) | ||
| ! Loop over each catchment and read: id, catchment id, left/right longitudes, bottom/top latitudes | ||
| do i = 1, ntot | ||
| read(77, *) tid(i), catid(i), lon_left(i), lon_right(i), lat_bottom(i), lat_top(i) | ||
| end do |
There was a problem hiding this comment.
This block looks like it should be a call to a reader for the catchment.def file. I thought we have a fortran reader for the catchment.def file, but I couldn't find one. (We do have a Matlab reader.) BUT see next comment
| latc = (lat_bottom + lat_top) / 2. | ||
| lonc = (lon_left + lon_right) / 2. |
There was a problem hiding this comment.
The catchment.def info is needed only to compute the center-of-mass lat/lon values for the tiles. However, this should be obtained from the tile file (*.til). Using the average of the min/max lat/lon isn't quite correct when there are non-land raster grid cells within the EASE grid cell.
PS: Not sure if the lat/lon values in the tile file are actually different. But if they're the same, they're also wrong in the tile file, and we shouldn't repeat the wrong calculations (and then risk that we will forget to fix them in both places if we ever decide to fix the issue)
| np.savetxt(loni09_output_file, loni09 + 1, fmt='%d') | ||
|
|
||
| # Compute global grid cell area | ||
| def area_global_rectilinear_grid(lat, lon, rearth=6371.22): |
There was a problem hiding this comment.
"rearth" is inconsistent with "MAPL_RADIUS = 6371.0E3" from shared/Constants/PhysicalConstants.F90, which is what GEOS uses. Unfortunately, making this change will result in non-zero-diff results
|
|
||
| contains | ||
|
|
||
| subroutine get_Pfaf_frac(file_out, BCS_PATH, GridName) |
There was a problem hiding this comment.
rename subroutine to something like EASE_get_Pfaf_frac() for clarity
|
@gmao-rreichle Thank you for all the comments and changes. I will review them and edit the branch accordingly. |
5 participants
Add river routing module for offline land modeling (GEOSldas). Replaces #1023
Left for future PRs:
Related PRs:
Testing:
List of tasks that remain to be addressed (in no particular order):
@YujiN, I am trying to bring back some codes in develop branch. The develop branch is not working as it is but I believe it would be more efficient. The runoff and the other variables are distributed as needed, which is way more efficient than allgatherV.