Semi-distributed version of airGR models

A semi-distributed version of the models present in airGr have been mentioned several times in the past and recently:

• requests from external/internal users
• possibility of discarding GRSD for having a more flexible and user-friendly semi-distributed modelling framework
• need for an HYDRO intern to have rapidly the possibility to model simple catchments with one or two upstream nested catchments.

We brainstormed (Olivier, Charles, Mamoun and myself) 2 days ago. Here is a summary of an option of semi-distribution of airGR models we could easily and somehow rapidly implement: Concept:

• the user would be supposed to know the dependency between catchments
• the user would implement a loop that browses the dependency tree
• for upstream catchments, run/calibrate normally the GR models
• for downstream catchments, run/calibrate GR models on the downstream contributive area only, and add the lagged upstream simulated discharge
• it means that for each subcatchment, specifying the specific InputsModel, RunOptions and CalibOptions would still be necessary

Implementation:

• Upgrade the CreateInputsModel function to add the following (optional for catchments that have upstream catchments): a matrix of upstream simulated discharges, a matrix of upstream areas + downstream area (to convert discharges from mm to m3/s), a matrix of upstream hydraulic lengths
• create a RunModel_SD (or preferably upgrade the RunModel function) that takes into account the upgraded InputsModel and a Param vector that also contains the LAG parameter: this RunModel_SD function would simply be the addition of downstream discharge and the convolution product between lag and upstream discharge (see code below)
• Upgrade the CreateCalibOptions function to add 1 to the number of parameters for the SD case, add quantiles and a transformation for this parameter and add a "SD" type
• Create a TransfoParam_Lag function (same transfo as X4?).

This options would work well for sequential calibration of catchments (not for conjoint calibration of clusters of catchments).

Old RunModel

function (InputsModel, RunOptions, Param, FUN_MOD) 
{
  FUN_MOD <- match.fun(FUN_MOD)
  return(FUN_MOD(InputsModel = InputsModel, RunOptions = RunOptions, 
                 Param = Param))
}

New RunModel

function (InputsModel, RunOptions, Param, FUN_MOD) 
{
  FUN_MOD <- match.fun(FUN_MOD)
  Q_down <- FUN_MOD(InputsModel = InputsModel, RunOptions = RunOptions, 
                    Param = Param)
  if (SEMI_Distributed) {
    Q_tot <- Q_down + SUM(of lagged (cf Param$LAG) upstream discharges given in InputsModel)
  } else {
    return(Q_down)
  }
}

added ENHANCEMENT R labels

changed the description

changed the description

I finally scanned my notes from the meeting.

scan_notes_mars2020.pdf

Attached is the code provided by Mamoun about his attempt to implement a SD airGR. Everything was in a single file, so I created one file per function + an example file. The code was not working (especially one CreateInputsModel check was badly implemented, I corrected that).

The option followed by Mamoun does not correspond what we discuted in the meeting regarding some parts. In particular, we want to limit the number of modifications and functions modified (for example, there is no reason to modify CreateRunOptions. However, I used some parts of the code to begin my own implementation. See next post.

TransfoParam_test.R

airGR_test.R

Calibration_Michel_test.R

CreateInputsModel_test.R

CreateCalibOptions_test.R

CreateRunOptions_test.R

RunModel_test.R

This post is just for information and to keep track. Do not implement please.

Olivier, attached is my proposition to run a SD version of any (daily or hourly) airGR model. I will work on calibration later on, after we validate this step.

CreateInputsModel_SD was modify to integrate the 3 new arguments from line 187. We have a new class, "SD". Olivier, please add some more checks on the type of data for the arguments. I think that a SD GR1A is not that useful. What about a SD GR2M model?

RunModel_SD runs normally if no "SD" class is present in InputsModel. Otherwise, the model is run for the downstream part and all discharges (4 upstream discharges + 1 downstream discharge) time series are summed proportionally to the surface. Upstream discharges are lagged. I'm not 100% sure of this calculation, some hands-on verification will be necessary. The OutputsModel object contains one more variable: Q_down, which corresponds to the downstream contribution to discharge. Qsim still is the total outlet discharge.

test_airGR_SD is to test the 2 new functions on a simple example.

CreateInputsModel_SD.R

RunModel_SD.R

test_airGR_SD.R

The inputs arguments have been renamed.

The current use of Param[-length(Param)] to handle the lag parameter in the Runmodel() function is dangerous. It will need to be changed.

Since we don't know in advance the number of parameters of the hydraulic model, it's dangerous indeed.

I search for a way to identify the number of parameters used by the model. After searching in all the code, I finally found that the number of parameters is always given in the name of the model function: RunModel_GR1A, RunModel_GR2M, RunModel_GR4H... If we continue to follow this rule in the future, we are able to get the number of parameter by parsing the name of the model function.

Another way to do that, is to add class affiliation in CreateInputsModel by adding for example "NParam2", "NParam3"... in class(InputsModel).

The latter has my preference since it doesn't rely on an implicit naming convention...

Which one do I implement?

In fact, with airGRplus which will have to manage other models, I don't see how to simply do without a table containing this information (see attached) and to manage it with a function like .TypeModelGR() (in airGRteaching)

> ModFeatures
            code          name nParam id  class      pckg
1           GR1A          GR1A      1 NA     GR     airGR
2           GR2M          GR2M      2 NA     GR     airGR
3           GR4J          GR4J      4 NA     GR     airGR
4           GR5J          GR5J      5 NA     GR     airGR
5           GR6J          GR6J      6 NA     GR     airGR
6           GR4H          GR4H      4 NA     GR     airGR
7           GR5H          GR5H      5 NA     GR     airGR
8  CemaNeigeGR4J CemaNeigeGR4J      6 NA     GR     airGR
9  CemaNeigeGR5J CemaNeigeGR5J      7 NA     GR     airGR
10 CemaNeigeGR6J CemaNeigeGR6J      8 NA     GR     airGR
11 CemaNeigeGR4H CemaNeigeGR4H      6 NA     GR     airGR
12 CemaNeigeGR5H CemaNeigeGR5H      7 NA     GR     airGR
13          TOPM      Topmodel      8  1 LibMod airGRplus
14          IHAC       IHACRES      6  2 LibMod airGRplus
17          HBV0           HBV      9  5 LibMod airGRplus
18          MOHY        Mohyse      7  6 LibMod airGRplus
20          MORD        Mordor      6  8 LibMod airGRplus
21          SACR    Sacramento     14  9 LibMod airGRplus
22          SIMH        Simhyd      8 10 LibMod airGRplus
23          SMAR          SMAR      9 11 LibMod airGRplus
24          TANK          TANK     10 12 LibMod airGRplus
25          HYMO         HYMOD      6 13 LibMod airGRplus
26          GARD      Gardenia      8 14 LibMod airGRplus
27          PDM0           PDM      8 15 LibMod airGRplus
28          CREC          CREC      8 16 LibMod airGRplus
29          CEQU       Cequeau      9 17 LibMod airGRplus
30          NAM0           NAM     10 18 LibMod airGRplus
31          WAGE    Wageningen      8 19 LibMod airGRplus
32          XINA    Xinanjiang     12 20 LibMod airGRplus

ModFeatures.rda

created branch sd to address this issue

mentioned in commit 1568a691

mentioned in commit 617bc09d

mentioned in commit fe2cc30d

mentioned in commit 329e4a8c

mentioned in commit 3921249e

mentioned in commit 2ff9e331

mentioned in commit 302875fa3e1703eb2d278e38fe7c1681eb009c6a

mentioned in commit a8569975b3961a52ce8d5eabca0ef5b8f4006c7b

mentioned in commit b5c91a87738d905eebd6547860427980fcd2c4dd

It may be a good idea to handle input arguments as in the CreateInputsCrit() function with Obs, VarObs = "Q", ..., using lists (and thus data frames) or vectors.

assigned to @david.dorchies

mentioned in commit 0478abdd

mentioned in commit 070e89e0

mentioned in commit c8036d7a

mentioned in commit e629e366

mentioned in commit be59893c

mentioned in commit e7857d90

mentioned in commit 7c130ef6

mentioned in commit 3b7bee28

mentioned in commit 89052828

I did the changes to allow for SD calibration (i.e. with a LAG).

I:

• created a TransfoParam_LAG function. So far it is done for the GRSD LAG function only, but in the future we could think of some more modularity
• modified the CreateCalibOptions and Calibration_Michel functions to allow for calibration of a SD model (actually a lumped model with a LAG component for upstream Q)
• adapted the NAMESPACE file
• did not modify the TransfoParam.Rd help file

@olivier.delaigue or @david.dorchies: I let you check and make the commits. I rebuilt and instlled the new package and it compiles. I also ran the CreateCalibOptions example (GR4J calibration) and a CemaNeigeGR4J calibration, they both work well, but maybe your automatic tests could be used to further test my implementation. I did not try a SD calibration: I'm waiting for data from a student to make a thorough comparison between GRSD and airGR-SD.

NAMESPACE

Calibration_Michel.R

TransfoParam_LAG.R

CreateCalibOptions.R

I took the modifications and I have rebased this branch to the dev one. I'll push "force" the branch if tests are OK.

mentioned in commit 17a3ee9c

mentioned in commit b67e59aa

mentioned in commit f51d6cdc

mentioned in commit 8958631d

mentioned in commit d3114b06

mentioned in commit dd7c0dbb

mentioned in commit a037e702

mentioned in commit 2590a4d2

mentioned in commit cc7433c3

mentioned in commit 52b3cb67

I have big doubt about the formula applied for the lag of the upstream tributaries in the current version of the code.

First, the contribution of the downstream sub basin is calculated respectively to its area.

$$Q_{sim} = \begin{bmatrix} Q_{down}(1) \\ Q_{down}(2) \\ ... \\ Q_{down}(k) \\ \end{bmatrix} \times S_{down} / S_{basin}$$

with $s_{down}$ downstream sub basin area and $S_{basin}$ the total area of the basin.

I'm OK until here.

We then add the contribution of upstream tributaries with a delay (lag) proportionally to the hydraulic distance between these tributaries and the basin outlet.

$$Q_{sim} = Q_{sim} + 1000 / P_{lag} / T_{s} \\ \times \\ \begin{bmatrix} Q_{up_1}(1) & Q_{up_2}(1) & ... & Q_{up_n}(1) \\ Q_{up_1}(2) & Q_{up_2}(2) & ... & Q_{up_n}(2) \\ ... & ... & ... & ... \\ Q_{up_1}(k) & Q_{up_2}(k) & ... & Q_{up_n}(k) \\ \end{bmatrix} \\ \times \\ \left( \begin{bmatrix} L_{1} \\ L_{2} \\ ... \\ L_{n}\\ \end{bmatrix} \times \begin{bmatrix} S_{up_1} \\ S_{up_2} \\ ... \\ S_{up_n}\\ \end{bmatrix} \right)$$

First, discharges are multiplied by each sub basin area (as in the first step) but not divided by the whole basin area. Second, I don't understand how the upstream discharges can be delayed with this operation involving a discharge multiplied by a length...

I think that the operation is more complex and need a loop for delaying each upstream discharge time series before summing all with the downstream contribution.

Hope that the current formula is not used elsewhere. Tell me if I'm wrong.

Thanks @david.dorchies I will analyse that further as soon as possible (next week?). I might have mistranslated some pieces of code from Mamoun, don't worry the GRSD formulation is ok!

Let:

• $C$, the lag parameter which is a celerity in m/s.
• $L_i$ the distance between the i^th tributary and the downstream outlet in m.

The delay time $\Delta t_i$ in seconds for the i^th upstream tributary will be:

$$\Delta t_i = \dfrac{L_i}{C}$$

The number of time steps $N$ to be shifted considering the time step $T_s$ in seconds of the model will be:

$$N = \dfrac{\Delta t_i}{T_s}$$

Considering that $N$ is not an integer, we need to shift the discharge between two time steps proportionally to the fractionnal part of $N$: $\{N\} = N - \lfloor N \rfloor$

The shifting formula is then as follow:

$$Q_{down}^i(t) = (1 - \{N\}) Q_{up}^i(t - \lfloor N \rfloor) + \{N\} Q_{up}^i(t - (\lfloor N \rfloor + 1))$$

The formula was indeed super wrong, sorry I just tried to translate a piece of code I as given without thinking really about it... Attached a new version. Tell me what you think about it.

Can you confirm to me that/when I can upload a new version of the package from the sd branch?

At some point, we will be missing the storage of upstream discharge not yet arrived to the outlet in the StateEnd data.

RunModel.R

Done in commit 88363e7e

Still not correct, working on it...

It runs now and results are quite good.

Newly modifed files below.

@david.dorchies if possible, you can try to redo the calibration / simulation with GR4J and CemaNeige on the Seine River in similar conditions that what you had for Climaware.

RunModel.R

Calibration_Michel.R

CreateCalibOptions.R

@guillaume.thirel, in RunModel(), the length_ts variable is not used. Is it normal?

I forgot to use it! Now it is used.

RunModel.R

Great, it's more like what I expected :)

I implement some simple tests to check if the lag is correctly implemented (test with a delay of one time step and another one with 1.5 time step).

After that, I'd like to progress in the idea to use RunModel as a chained model scheduler. Can you give your first impressions on the monologue below? I think that this idea to give a list of model and a list of parameters to RunModel would allow us to treat even complex semi-distributive water basin in one call.

Guillaume says below:

It inspires to me that this is a potentially (very) good idea. But I would like first to validate and crash-test the new SD version.

We found that the code was very likely wrong, as upstream discharge was not delayed, but first time steps were erased. What do you think about the hopefully more correct code below?

      OutputsModelDown$Qsim <- OutputsModelDown$Qsim +
        c(rep(0, floor(PT[upstream_basin])),
          Qupstream[1:(LengthTs - floor(PT[upstream_basin]))]) *
        HUTRANS[1, upstream_basin] +
        c(rep(0, floor(PT[upstream_basin] + 1)),
          Qupstream[1:(LengthTs - floor(PT[upstream_basin])-1)]) *
        HUTRANS[2, upstream_basin]
      # OutputsModelDown$Qsim <- OutputsModelDown$Qsim +
      #   c(rep(0, floor(PT[upstream_basin])),
      #     Qupstream[(1 + floor(PT[upstream_basin])):LengthTs]) *
      #   HUTRANS[1, upstream_basin] +
      #   c(rep(0, floor(PT[upstream_basin] + 1)),
      #     Qupstream[(2 + floor(PT[upstream_basin])):LengthTs]) *
      #   HUTRANS[2, upstream_basin]

OMG! You're right!

I'm also very afraid that the tests I wrote didn't detect this lamentable error I check why the tests didn't reveal this error. And correct that.

That should explain why I had very weird lag parameters on Seine calibration so far.

The tests were written by the same person who the code wrote the code... I better understand the interest of binomial test-driven development (https://fr.wikipedia.org/wiki/Test_driven_development#Programmation_binomiale_en_TDD).

I have adopted the correction proposed by @guillaume.thirel in commit 3dd37b5e with the related tests.

In these tests, I do a test of calibration by adding an artificial upstream flow to the vignette 1 example. I had to choose an upstream flow with a strong signal (i.e. a cycle consisting of constant flows with quick variations) in order to get a good identification of the original GR4J parameters found in vignette 1.

mentioned in merge request !10 (merged)

I've created a work in progress merge request of this issue !10 (merged) in order to have an extensive overview of code changes.

I'm afraid that adding a new step of simulation implies distributive modifications. I explain below: First we have a family of rainfall-runoff models models:

graph TD

PE[(PE)]

PE --> GR4J
PE --> GR5J
PE --> GR6J

Q[(Q)]

GR4J --> Q
GR5J --> Q
GR6J --> Q

And then, we have a preprocessing of the rainfall for the snow called:

graph TD

PE[(PE)]
PE --> Cemaneige
P'E[(P'E)]
Cemaneige --> P'E

P'E --> GR4J
P'E --> GR5J
P'E --> GR6J

Q[(Q)]
GR4J --> Q
GR5J --> Q
GR6J --> Q

But the current implementation looks like this:

graph TD

PE[(PE)]
PE --> GR4J
PE --> GR5J
PE --> GR6J
PE --> GR4J_Cemaneige
PE --> GR5J_Cemaneige
PE --> GR6J_Cemaneige

Q[(Q)]
GR4J --> Q
GR5J --> Q
GR6J --> Q
GR4J_Cemaneige --> Q
GR5J_Cemaneige --> Q
GR6J_Cemaneige --> Q

A lot of code is copy and paste in a multiplication of functions.

The implementation of the semi-distributive model should lead to:

graph TD

PE[(PE)]
PE --> Cemaneige
P'E[(P'E)]
Cemaneige --> P'E

P'E --> GR4J
P'E --> GR5J
P'E --> GR6J

Q[(Q)]
GR4J --> Q
GR5J --> Q
GR6J --> Q

Q --> LAG
Q --> LR2

Q'[(Q')]
LAG --> Q'
LR2 --> Q'

Now in !10 (merged) for the implementation of the semi-distributive model, I see a lot of code with repetitive conditions taking into account the using of either or both SD and Hysteresis and I'm afraid about the entanglement of all of this.

Should we not consider all of that as a model chaining? At each step of the chain, the model takes input time series and return as output transformed and/or supplemented data ? Then, in our procedures we'll be able to use an ordered list of model to run instead of a composite model of all the possible combinations. For example, running Cemaneige-GR4J-SD could looks like this:

graph TD

D1[(P, E, T, Qup1, Qup2)]

D2[(P, E, T, Qup1, Qup2, Ptot)]

D1 --> Cemaneige

Cemaneige --> D2

D3[(P, E, T, Qup1, Qup2, Ptot, Q)]

D2 --> GR4J
GR4J --> D3

D3 --> LAG

D4[(P, E, T, Qup1, Qup2, Ptot, Qdown, Q)]

LAG --> D4

Please note that:

• P in Cemaneige output is the transformed one and Ptot the original one,
• Qdown in LAG output is equal to its Q input and the Q ouput is the one summing Qup1, Qup2 and Qdown.

The calling of RunModel can then look like this:

OutputsModel <- RunModel(InputsModel, RunOptions, Param, list('Cemaneige', 'GR4J', 'LAG'))

And in RunModel, we just have a loop on the models taking the Ouput of the previous model as inputs of the following one. I think that we can implement this without modifying current interfaces of airGR (For example, we can rewrite RunModel_CemaNeigeGR4H with just a call to RunModel with FUN_MOD = list('GR4J', 'Cemaneige') which will not change anything for the final user.

By the way, param should be a list of vector to simplify all the thing: param = list(GR4J = c(X1, X2, X3, X4), LAG = celerity)

Tell me what this reflection inspires to you :)

It inspires to me that this is a potentially (very) good idea. But I would like first to validate and crash-test the new SD version.

Maybe put that in another issue to avoid having too much here?

I think it's a very good idea. But maybe it's safer not to change everything at once. On the other hand, we also had the idea to avoid duplicating code in all the RunModel*() function by doing something similar to what was done with the ErrorCrit*() functions where the problem of duplicated code was similar. And the RunModel*() functions have to be cleaned before that... #14 (closed)

OK let's checking the robustness of this first version of SD model, first. And develop this idea of model chaining in another ticket.

mentioned in commit 88363e7e

mentioned in commit 61d9a74a

mentioned in commit 5083d45a

mentioned in commit 31352a7b

mentioned in commit 9cb63cfd

mentioned in commit 031b0716

mentioned in commit 9a99a6c5

First tests with one upstream basin are conclusive.

Calibration of the lag parameter is very sensitive to the flow contribution of each basin. A first test with a lag of one time step (~0.11 m/s) and same flow and surface for both basins has been calibrated at ~0.5 m/s. A second test with an upstream basin surface area double that of the downstream basin leads to the good lag value.

Adding constraint to GR4J parameters calibration of the downstream knowing upstream GR4J parameters could help in this case.

mentioned in issue #14 (closed)

I'm facing an issue with the unity used for flow in the current implementation of sd model in airGR. Currently, we use the water height and we need to make a conversion proportional to the area of the sub-basin for computing the flow at the outlet of a basin. Comparatively to directly using a volume as flow unit, we are spending a lot a computation time to multiplying and dividing instead of just adding to flow. But it's not the main point I want to mention here.

With the current implementation of the lag model, we are not able to directly integrate inflows (reservoir releases) or outflows (reservoir or users withdrawals) which are not related to a basin area. For example, it is not possible to calibrate a influenced model by directly integrating intakes and outlets in the basin as on the scheme below:

I think that it would be advantageous to convert once the output of the hydrological model into volume and all the calculation after would be greatly simplified.

I understand that water height is very useful in hydrological paradigm but when we are addressing water management everything is related to volumes.

Another advantage would be that the vector of surface areas of upstream basins given in InputsModel would be useless. Only the area of the downstream sub-basin would be necessary for its conversion into a volume.

I'm facing an issue with the unity used for flow in the current implementation of sd model in airGR. Currently, we use the water height and we need to make a conversion proportional to the area of the sub-basin for computing the flow at the outlet of a basin. Comparatively to directly using a volume as flow unit, we are spending a lot a computation time to multiplying and dividing instead of just adding to flow. But it's not the main point I want to mention here.

GR models are made to work on mm per time step, units conversion will always be necessary, we cannot change that.

GR models are made to work on mm per time step, units conversion will always be necessary, we cannot change that.

Yes, I know that. But We have to find a way to inflow or outflow in the sub-basin that are not related to a surface. The current implementation assumes that all upstream flows are related to a surface so we can't introduce a point in the middle of the sub-basin where a discharge is ponctualy provided (e.g.: outlet of a reservoir or a waste water treatment plant) or taken (e.g.: withdrawals for industry, irrigation or drinking water).

We need a parameter specifying the nature of the upstream flow for differencing run-off from specific point of flow exchange with the river.

I see your point now. Modifying the actual unit of the output of GR models is not an option but we have to find a way to take into account flows in m3/s indeed.

I propose this convention:

If the area provided for an upstream basin is NA, then we assume that the corresponding upstream discharge is in m3/s. Otherwise, if the area is positive, we assume that the upstream flow is in mm as usual. And I propose to add a warning in CreateInputsModel in case of NA upstream basin area in order to report that the corresponding column of Qupstream will be considered in m3/s.

Is this OK for you?

I tend to agree, yes. I keep the possibility to change my mind later of course. :)

BasinArea needs to be set as optional in the doc and some explanation given.

OK, let's go with this idea for the moment.

I will complete the documentation when all these choices will be stabilised and implemented.

mentioned in commit 65c221ea

Question about parameters of CreateInputsModel: why LengthHydro and BasinAreas must be matrices of numeric values with 1 row while these data has only one dimension?

It's a bit painful to fill this requirement for the final user. It forces him to write this kind of thing:

  LengthHydro = matrix(LengthHydro , nrow = 1),
  BasinAreas = matrix(BasinAreas , nrow = 1)

I let @olivier.delaigue answer this one, as we need to be consistent with similar variables used in airGR.

Because @guillaume.thirel wrote that and I didn't have time to change it! I think that we just need vectors.

Cool! So I change it!

I have one suggestion: as upstream flows informations are multiple, if think it could be more coherent to ask the user to provide a dataframe with consistent information for each upstream flow. This dataframe (which can be called UpstreamParam) should at least contains two columns: HydroLength and BasinAreas.

Associated with a parameter DownstrArea in InputsModel for the downstream area, this would simplify all the checking on the number of items or columns in LenghtHydro, BasinAreas and QobsUpstr.

The dataframe form should also have the advantage of opening the possibility to had parameters associated with upstream flows without having to add new parameters in CreateInputsModel. I am thinking in particular of extra parameters needed to describe special upstream flows such as reservoirs and withdrawals.

I have also have a suggestion for the name of QobsUpstr which I think should be called Qupstream because the upstream flow is not necessary an observation but is more likely a simulated one.

I have also have a suggestion for the name of QobsUpstr which I think should be called Qupstream because the upstream flow is not necessary an observation but is more likely a simulated one.

Definitely ok with this one.

Regarding the other questions, I would say ok but maybe we should discuss that to be sure how this could impact the rest.

I'm not sure that using a data.frame as an input is a standard and recommended practice (if the user is not careful, he can invert the columns)...

Other question: why use kilometer as unit for LenghtHydro if we need to multiply it by 1000 each time we calculate PT in RunModel?

It could be useful to directly use meter unit as parameter of the model.

It's ok for me to use m instead of km and remove the 1000 multiplication. Maybe please add in the doc Rd file some info about necessary inputs (and related units).

OK. I'll do it.

mentioned in commit 088a5ddb

When we don't use the SD model, in verbose mode (which is the default), this warning is displayed:

Missing argument: 'Qupstream', 'LengthHydro' and 'BasinAreas' must all be set to run in a semi-distributed mode. The lumped mode will be used

I think that this message will be confusing for the users especially the beginners because using SD model is the exception and the main use is the lumped model.

Considering that I see 2 alternatives:

• Remove the warning message
• Display a warning message in the special case of activating the SD model

What do you think?

This issue happens in the CreateInputsModel function.

Alternative 3: this check should rather be something that returns a warning in case one of c('Qupstream', 'LengthHydro') arguments is not given (but the other one is). I propose reformulating the argument as follows:

Missing argument: both 'Qupstream' and 'LengthHydro' must be set to run in a semi-distributed mode. As a consequence, the lumped mode will be used here

Oh! My bad! I misunderstood the code. This message is already displayed if one of the requested parameters for SD is missingd AND at least one SD parameter is present. https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/blob/sd/R/CreateInputsModel.R#L191

Actually, I think that this warning should be an error message. The user gives some but not all parameters for SD model and the model runs anyway but in lumped mode. Moreover, if the verbose mode is inactivated, this happens silently. I think this is a big source of error and misunderstanding for the user.

When we don't use the SD model, in verbose mode (which is the default), this warning is displayed:

Oh! My bad! I misunderstood the code.

Ok I'm lost now, is this message displayed when the lumped mode is run or not?

I agree that this could be an error message.

mentioned in commit 67ce1ead

mentioned in commit e974451b

changed milestone to %v1.6.10

mentioned in commit baffd2ea

mentioned in commit 0cdd8938

mentioned in commit 811200e3

mentioned in commit 3dd37b5e

mentioned in commit db0de232

mentioned in commit 8534b839

mentioned in commit eb954b99

mentioned in commit ad63d07c

mentioned in commit 6c2eb581

mentioned in commit 2ae49608

mentioned in commit b4287b28

mentioned in commit 20976819

mentioned in commit ec243d37

mentioned in commit 5a298fbc

mentioned in commit ea066eb4

mentioned in commit 9cb45ef6

mentioned in commit 3433229e

closed via merge request !10 (merged)

closed via commit 2590a4d2

mentioned in commit d5ac0907