included soil ODE scheme. Little issue in GW mass balance #3
Conversation
There was a problem hiding this comment.
Thanks, this is great. I ran the code and it works. The main thing is that this functionality has to be an option, but replacing the old code. Even though the old code has problems, there is an important aspect of Nextgen to be able to run models as the origional author intended, even if it is a mistake, but we'll just have the option to run the soil-ode instead, and we'll run that most times.
So basically we need a setup like the partitioning scheme can be either Xinanjiang or Schaake. We'll need a soil reservoir flux option for "ODE". I can help with this, if you want.
| @@ -368,14 +379,14 @@ def finalize_mass_balance(self, verbose=True): | |||
|
|
|||
| self.vol_soil_end = self.soil_reservoir['storage_m'] | |||
|
|
|||
| self.global_residual = self.volstart + self.volin - self.volout - self.volend -self.vol_end_giuh | |||
| self.schaake_residual = self.volin - self.vol_sch_runoff - self.vol_sch_infilt - self.vol_et_from_rain | |||
| self.global_residual = self.volstart + self.volin - self.volout - self.volend -self.vol_end_giuh #(?) - vol_in_nash_end? --- This is already excluded from the system. | |||
There was a problem hiding this comment.
I'm not 100% sure, but I think this is still needed here. Since the global residual needs to account for anything that can be left over. Unless you are sure that it will be zero at any time step, we should still subtract from volstart and volin.
| @@ -233,7 +240,7 @@ def initialize(self,current_time_step=0): | |||
|
|
|||
| # ________________________________________________ | |||
| # Nash cascade | |||
| self.K_nash = 0.03 | |||
| self.K_nash = self.K_nash | |||
| import math | ||
|
|
||
| @jit(nopython=True) | ||
| def conceptual_reservoir_flux_calc(t, S, storage_threshold_primary_m, storage_max_m, coeff_primary, coeff_secondary, PET, infilt, wltsmc): |
There was a problem hiding this comment.
Why not include this in the CFE class? Then all these inputs will be in CFE_STATE.
There was a problem hiding this comment.
I simply didn't try! They can be included in the CFE class
| cfe_state.volout += cfe_state.actual_et_from_rain_m_per_timestep | ||
|
|
||
| # ________________________________________________ | ||
| # Calculate evaporation from soil |
There was a problem hiding this comment.
We need to include options for all the old code. We have to be able to run an identical version as the C code. So there needs to be an option from the configuration file to run this.
|
Thank you so much for the review! If you could help me with separating ODE option, it would be great! And I can go ahead figuring out the GW mass balance issue. Do all changes need to be solved and the PR needs to be merged before you work on the ODE option? Then I will prioritize it (probably done by EOD today or tomorrow) |
|
I can make a new branch and implement the option in the configuration file.
Then I'll share that with you and you can decide implement if you want or
re-do it.
…On Thu, Jun 15, 2023 at 10:17 AM Ryoko Araki ***@***.***> wrote:
Thank you so much for the review!
If you could help me with separating ODE option, it would be great! And I
can go ahead figuring out the GW mass balance issue.
Do all changes need to be solved and the PR needs to be merged before you
work on the ODE option? Then I will prioritize it (probably done by EOD
today or tomorrow)
—
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKP6YQNV7YK7W52COCWAO7DXLMRRJANCNFSM6AAAAAAZCRBINA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
Sounds good!
sent from mobile
On Thu, Jun 15, 2023, 10:40 AM Jonathan Frame ***@***.***>
wrote:
… I can make a new branch and implement the option in the configuration
file.
Then I'll share that with you and you can decide implement if you want or
re-do it.
On Thu, Jun 15, 2023 at 10:17 AM Ryoko Araki ***@***.***>
wrote:
> Thank you so much for the review!
>
> If you could help me with separating ODE option, it would be great! And
I
> can go ahead figuring out the GW mass balance issue.
>
> Do all changes need to be solved and the PR needs to be merged before
you
> work on the ODE option? Then I will prioritize it (probably done by EOD
> today or tomorrow)
>
> —
> Reply to this email directly, view it on GitHub
> <
#3 (comment)>,
> or unsubscribe
> <
https://github.com/notifications/unsubscribe-auth/AKP6YQNV7YK7W52COCWAO7DXLMRRJANCNFSM6AAAAAAZCRBINA>
> .
> You are receiving this because you commented.Message ID:
> ***@***.***>
>
—
Reply to this email directly, view it on GitHub
<#3 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AMNGL2GF6KN2WFBBTJOSYXDXLMUH5ANCNFSM6AAAAAAZCRBINA>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
Summary
This PR addresses the issue NOAA-OWP/cfe#82
Note that this PR will solve the issue above, but changes the model behavior and scheme largely, and increase the computational time.
Known issue
This PR still has a little issue in groundwater mass balance in this PR. Refer to the code https://github.com/RY4GIT/SMSigxModel/tree/mcmillan_ws/py_cfe or NOAA-OWP/cfe#66 or , which don't have groundwater mass balance issues. I probably forgot to copy or delete some lines from those versions to this one.