pywr / pycatchmod

A Cython implementation of the rainfall runoff model CATCHMOD (Wilby, 1994)
GNU General Public License v3.0
6 stars 5 forks source link

Nonlinear storage constant == 0 #11

Closed snorfalorpagus closed 7 years ago

snorfalorpagus commented 8 years ago

Just come across this error. I think that 0 is a valid value of Cq?

---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-20-f088b307b897> in <module>()
----> 1 C = catchment_from_json("Catchmod (updated)/Bewl.json", n=1)

d:\pycatchmod\pycatchmod\utils.py in catchment_from_json(filename, n)
     22             gradient_drying_curve=subdata['gradient_drying_curve'],
     23             linear_storage_constant=subdata['linear_storage_constant'],
---> 24             nonlinear_storage_constant=subdata['nonlinear_storage_constant']
     25         ))
     26 

d:\pycatchmod\pycatchmod\_catchmod.pyx in pycatchmod._catchmod.SubCatchment.__init__ (pycatchmod\_catchmod.c:6341)()
    251 
    252         self._nonlinear.step(outflow, outflow)
--> 253         return 0
    254 
    255     property size:

d:\pycatchmod\pycatchmod\_catchmod.pyx in pycatchmod._catchmod.NonLinearStore.__init__ (pycatchmod\_catchmod.c:4851)()
    170         for i in range(n):
    171 
--> 172             if self.previous_outflow[i] > 0.0:
    173                 if inflow[i] < -ZERO:
    174                     # Case (b)

ValueError: Invalid value for nonlinear storage constant. Must be > 0.0
jetuk commented 8 years ago

Well it causes a bin of divide by zero errors. I assume Cq == 0 is equivalent to no non-linear storage component. Probably better to have Cq == None and not use the non-linear component?

snorfalorpagus commented 8 years ago

The Excel models states "Typically 0 to 5000" next to the input. A value of 0 effectively means there is not output from the nonlinear storage, i.e. the subcatchment has a linear component only.

jetuk commented 8 years ago

Hence keep 0 as an invalid input NonLinearStore, but if None is given then don't make one for that subcatchment?

snorfalorpagus commented 7 years ago

I'm happy to convert the value to None and skip the nonlinear storage if a zero value is given.

I've identified another bug, whereby when the nonlinear storage is None then the averaging between yesterday's flow and today's flow isn't done. I think this needs to be done as part of the SubCatchment object, not the NonLinearStorage. This creates a small problem, as the nonlinear storage needs access to the previous flow.

jetuk commented 7 years ago

I think it best if both storage objects keep a previous flow (which they do now), but also have an property to produce the daily average if required. Then you would have the following pseudo code in SubCatchment,

if non_linear_store:
     Q = non_linear_store.daily_average_flow
else:
     Q = linear_store.daily_average_flow
snorfalorpagus commented 7 years ago

This was resolved in d7a831ba5362209876ed3e171294a1fab3ff97a7.