Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Factor out the runoff concentration process into new submodels #130

Closed
Tracked by #122
parvumflumen opened this issue Mar 12, 2024 · 8 comments
Closed
Tracked by #122

Factor out the runoff concentration process into new submodels #130

parvumflumen opened this issue Mar 12, 2024 · 8 comments

Comments

@parvumflumen
Copy link
Collaborator

parvumflumen commented Mar 12, 2024

The proposed naming of the HydPy-H application models would require factoring out the runoff concentration process (triangular uni hydrograph or linear storage cascade) into new submodels. The implementation effort should be manageable compared to other submodularisation efforts, but we should anticipate at least two days of work (primarily for documentation updates). factor out the runoff concentration process into new submodels

@parvumflumen parvumflumen mentioned this issue Mar 12, 2024
30 tasks
@parvumflumen parvumflumen changed the title ...The proposed naming of the HydPy-H application models would require factoring out the runoff concentration process (triangular uni hydrograph or linear storage cascade) into new submodels. The implementation effort should be manageable compared to other submodularisation efforts, but we should anticipate at least two days of work (primarily for documentation updates). factor out the runoff concentration process into new submodels Mar 12, 2024
@parvumflumen parvumflumen changed the title factor out the runoff concentration process into new submodels Factor out the runoff concentration process into new submodels Mar 19, 2024
@parvumflumen
Copy link
Collaborator Author

For the submodel, the following designations have been preliminarily chosen:

  • name of the submodel: rconc
  • name of the application models: rconc_convolution and rconc_storagecascade
  • function that calculates the unit hydrograph output: Calc_OutUH_QUH_V1 (unchanged)
  • function that calculates the storage cascade output: Calc_OutUH_SC_V1 (unchanged)
  • control parameters: base length TB (former MaxBaz), NmbStorages (unchanged)
  • derived parameters: UH, KSC, DT (unchanged)
  • fluxes: InUH, OutUH (unchanged)
  • log sequences: QUH (unchanged)
  • state sequences: SC (unchanged)
  • interface functions: Set_RConcInput_V1, Determine_RConcOutput_V1, Get_RConcOutput_V1, Get_WaterBalance_V1, Get_WaterBalance_V2

Feel free to suggest any improvements!

@tyralla
Copy link
Member

tyralla commented Apr 15, 2024

@parvumflumen is currently working on this in #133

@tyralla
Copy link
Member

tyralla commented Apr 15, 2024

Feel free to suggest any improvements!

Thanks for the suggestions. Here are my thoughts on the model names.

I like the general name rconc. The only limitation I see is that the implemented methods are very general and can be applied on other properties than runoff as well. However, that's unlikely within HydPy, and if such a case occurs someday, we can add a note to the documentation.

Compared to the other user-relevant models' names, rconc_storagecascade is relatively long. We could abbreviate it to rconc_lsc ("l" for linear). Another idea: rconc_casc.

I am unsure how widely known the term "convolution" is. The apparent alternative is rconc_unithydrograph or, abbreviated, rconc_uh.

We should also clarify if the discussed submodel is only supposed to handle HBV96-like unit hydrographs or those of different shapes. In the first case, we should probably add "hbv96" to the name, like in evap_pet_hbv96 and evap_aet_hbv96.
In the second case, we should eventually try to follow the logic of musk_classic, which allows for HBV96-like Muskingum routing as well as "normal" Muskingum routing. This decision is related to the question of whether to also extract the unit hydrograph of GRxJ (@sivogel). Would this require a third rconc application model (rconc_lsc, rconc_uh_hbv96, and rconc_uh_gr), or should rconc_uh support HBV96- and GRxJ-style configuration?

@tyralla
Copy link
Member

tyralla commented Apr 15, 2024

Feel free to suggest any improvements!

My additional thoughts:

Regarding the interface methods: set_inflow, determine_outflow, and get_outflow instead of set_rconcinput, determine_rconcoutput, and get_rconcoutput?

From the perspective of hland: InUH and OutUH have never been appropriate names for hland_v3 and hland_v4. And, if I remember correctly, they are not official HBV96 terms. This may be the right opportunity to change them. Maybe QIn and QOut? These names would not be overly descriptive but fit the other short HBV names.

From the perspective of rconc: Combining the first two ideas, we could rename InUH and OutUH to Inflow and Outflow.

Regarding the other names: When not borrowing names of other models, we usually tend to use longer, more descriptive names. Could you make some suggestions that follow this spirit? But maybe we should wait with these details until the other decisions have been made. (For example, a parameter named MaxBaz would fit to rconc_uh_hbv96 whereas rconc_uh would require a more general parameter like TB - maybe with an optional keyword named maxbaz.)

@tyralla
Copy link
Member

tyralla commented Apr 22, 2024

We had an offline discussion about the base model name rconc, which would fit well with hland but not so well with gland (unit-hydrograph convolution of two intermediate fluxes, not of the "final" runoff). However, I cannot remember we found a better alternative.

We decided on the suffixes uh and nash for the application models. x_uh should be configurable in the style of HBV96 and GR4J. We agreed on doing this via keywords. One idea is to turn the derived parameter UH into a control parameter and move the functionality of its update method to its __call__ method. Then, one could use it like this:

uh(maxbaz=2.0)

Or we could define a new parameter like ConcentrationTime and UH would have to take the used keyword into account when calculating its ordinates:

concentrationtime(hbv96=2.0)

The second approach looks a little more understandable to me, but the first one offers the chance to define all ordinates freely by passing their values in a vector.

Also, we considered using less model-specific and more geometric keywords like "isosceles_triangle" but were unsure if short geometric terms could describe the GR4J-like unit hydrographs.

@tyralla
Copy link
Member

tyralla commented Apr 23, 2024

As a side-topic in a thorough discussion on finishing the GR implementation, we decided on the following.

From the perspective of a "pure" GR model, one should be able to add both unit hydrographs individually like this:

with model.add_rconcmodel1_v1("rconc_uh"):
    uh("gr_uh1", x4=2.0)
with model.add_rconcmodel2_v1("rconc_uh"):
    uh(option="gr_uh2", x4=2.0)

Then, x4, which is no longer part of GR, still appears in the control file. One can pass the first "UH type" parameter as a positional or keyword argument ("optional"), the others only as keyword arguments (that depend on the first argument, which should be typed with Literal).

We decided on gr_uh1 and gr_uh2 because both UH types are very model-specific. Instead, HBV96's triangle unit hydrograph appears a little more general. Hence, from the perspective of a "pure" HBV96 model:

with model.add_rconcmodel_v1("rconc_uh"):
    uh("triangle", tp=1.0, tb=2.0)

tb corresponds to maxbaz. tp (time to peak) is optional and is set to $tp = tb / 2$ by default (symmetric triangle unit hydrograph as used by HBV96).

We also discussed whether expecting the user to select two unit hydrograph submodels is too much. Still, we found no convincing alternative that would not lead to less flexibility or more complexity.

We strive to allow specifying the ordinates directly:

with model.add_rconcmodel_v1("rconc_uh"):
    uh([0.3, 0.5, 0.2])

@tyralla
Copy link
Member

tyralla commented Apr 23, 2024

When separating the different ways to calculate the UH ordinates, it could make sense to make new subclasses of IUH.

If we follow this strategy, we could eventually also support rconc-based unit hydrographs following the shapes of TranslationDiffusionEquation and LinearStorageCascade with very little effort:

with model.add_rconcmodel1_v1("rconc_uh"):
    uh("tde", u=5.0, d=15.0, x=50.0)
with model.add_rconcmodel2_v1("rconc_uh"):
    uh("lsc", n=2.5, k=2.0)

@tyralla
Copy link
Member

tyralla commented May 22, 2024

#133 has landed

@tyralla tyralla closed this as completed May 22, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants