search

E3SM-Project / E3SM

Energy Exascale Earth System Model source code. NOTE: use "maint" branches for your work. Head of master is not validated.
https://docs.e3sm.org/E3SM
Other
354 stars 368 forks source link

MOSART-sediment traps sediment in reservoirs before their build year #6309

Open tanzeli1982 opened 8 months ago

tanzeli1982 commented 8 months ago

I and @hydrotian found the issue described in the title. The following fix may work.

--- a/components/mosart/src/riverroute/MOSART_RES_type.F90 +++ b/components/mosart/src/riverroute/MOSART_RES_type.F90 @@ -22,7 +22,7 @@ module MOSART_RES_type use shr_sys_mod , only : shr_sys_flush, shr_sys_abort use netcdf use pio

liho745 commented 8 months ago

@tanzeli1982 @hydrotian I could be wrong, but my impression is that MOSART_sediment was integrated before adding the dam_built_year capacity, i.e., when there was no specification of reservoir construction years. In any case, I can certainly give it a try and fix this issue.

hydrotian commented 8 months ago

@liho745 I think you are right. DamConstructionFlag was implemented after the initial MOSART_sediment integration to master branch. Before The dam_built_year was in the WM parameter file already but never been really used.

liho745 commented 7 months ago

I've fixed this bug on a branch created off the E3SM master. https://github.com/liho745/E3SM/tree/liho745/river/bug-fix-no-sed-trapping-before-dam-construction. I did not do it on a branch created off the commit introducing MOSART-sediment or dam construction flag, mainly because 1) this bug does not affect any water cycle of BGC cycle campaigns and 2) the MOSART_developer tests are not available on those commits two-year-old. MOSART_developer tests passed on Compy BFB.

@hydrotian or @tanzeli1982 do you want to test the science of this bug fix before I issue a PR?

tanzeli1982 commented 7 months ago

I've fixed this bug on a branch created off the E3SM master. https://github.com/liho745/E3SM/tree/liho745/river/bug-fix-no-sed-trapping-before-dam-construction. I did not do it on a branch created off the commit introducing MOSART-sediment or dam construction flag, mainly because 1) this bug does not affect any water cycle of BGC cycle campaigns and 2) the MOSART_developer tests are not available on those commits two-year-old. MOSART_developer tests passed on Compy BFB.

@hydrotian or @tanzeli1982 do you want to test the science of this bug fix before I issue a PR?

@liho745, I and @hydrotian found that in riverroute/RtmMod.F90, MOSART_reservoir_sed_init() is called before MOSART restart file is read. As a result, when MOSART_reservoir_sed_init() is called, StorWater%active_stage is always zero. To avoid the issue, MOSART_reservoir_sed_init() should be moved downward in riverroute/RtmMod.F90.

hydrotian commented 7 months ago

@liho745 @tanzeli1982 Another way is to add the StorWater%active_stage calculation into WRM_init, because WRM_init is called before MOSART_reservoir_sed_init. This might be a cleaner solution. Let me know what you think and I can make some changes.

liho745 commented 7 months ago

@tanzeli1982 Nice thoughts. I am wondering whether this active_stage value will change during a simulation period which happens to include some dam construction year. In that case, this value should be checked and reset every model year, instead of being set once for all.

hydrotian commented 7 months ago

@liho745 The active_stage calculates every month. So I was proposing adding the calculation to WRM_init, not moving it entirely into WRM_init. But it may still have potential to create issues with restart from the middle of the month. Note that the active_stage has three values: 0 (before construction), 1 (filling stage), and 2 (fully functional). The active_stage (whether it's filling or fully functional) calculated from WRM_init based on current reservoir storage may be different from the values from the restart file, which is based on the storage from the first day of the month. Need further testing.

I optimistically think it should be fine as the sequence of the calculations would be WRM_init (get current active_stage) -> MOSART_reservoir_sed_init (determine trapping based on active_stage) -> RtmRestFileRead (get active_stage from restart file). Even active_stage may change but it could only change between 1 and 2 for the same year, which doesn't affect the sediment trapping.

tanzeli1982 commented 7 months ago

@tanzeli1982 Nice thoughts. I am wondering whether this active_stage value will change during a simulation period which happens to include some dam construction year. In that case, this value should be checked and reset every model year, instead of being set once for all.

Good thought. I agree that this flag needs to be checked every year.

liho745 commented 7 months ago

@hydrotian Overall, I agree with you. I am wondering whether you want to comment off Line 1914-1916 in the restart block. That restart block (Line 1976-1903) is supposed to be activated only once during the whole simulation. But Line 1914-1916 kind of forces WRM_computeRelease() to be called each time after initialization, hence overriding active_stage setting up during the initialization.