airGR issueshttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues2021-03-23T16:55:24+01:00https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/105Using wrong precipitation inputs when calling GR5H in CemaNeigeGR5H ?2021-03-23T16:55:24+01:00François BourginUsing wrong precipitation inputs when calling GR5H in CemaNeigeGR5H ?I was trying to understand why CemaNeigeGR5H was not as good as CemaNeigeGR4H on my case study and I found a difference in the two calls for .Fortran functions (``frun_gr4h`` and ``frun_gr5h``):
In CemaNeigeGR4H:
``InputsPrecip = CatchM...I was trying to understand why CemaNeigeGR5H was not as good as CemaNeigeGR4H on my case study and I found a difference in the two calls for .Fortran functions (``frun_gr4h`` and ``frun_gr5h``):
In CemaNeigeGR4H:
``InputsPrecip = CatchMeltAndPliq``
In CemaNeigeGR5H:
``InputsPrecip = InputsModel$Precip[IndPeriod1]``
Does it mean that the snow melt is not taken into account in CemaNeigeGR5H ?
CemaNeigeGR5J looks fine.v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/99Example of RunModel_Lag to improve2021-03-23T16:55:29+01:00Thirel GuillaumeExample of RunModel_Lag to improveWhile `LengthHydro` is supposed to be in m (see help of `CreateInputsModel`), the example of `Runmodel_Lag` gives a value of 150 **km**.
In addition, I don't see any difference in the result when I multiply the Lag parameter by 1000. I...While `LengthHydro` is supposed to be in m (see help of `CreateInputsModel`), the example of `Runmodel_Lag` gives a value of 150 **km**.
In addition, I don't see any difference in the result when I multiply the Lag parameter by 1000. Is that normal?v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/107RunModel_Lag: Unclear explanation of Lag2021-03-24T11:57:06+01:00Thirel GuillaumeRunModel_Lag: Unclear explanation of LagBelow an email sent by @cyril.thebault
> Selon moi, le Lag correspond au temps que met le débit pour se propager de l'amont vers l'aval : Lag [s] = LengthHydro [m] / v [m/s] (avec v la vitesse d'écoulement)
> Or dans Run_ModelLag le pa...Below an email sent by @cyril.thebault
> Selon moi, le Lag correspond au temps que met le débit pour se propager de l'amont vers l'aval : Lag [s] = LengthHydro [m] / v [m/s] (avec v la vitesse d'écoulement)
> Or dans Run_ModelLag le paramètre "Lag" qui est requis correspond au paramètre que j'ai appelé v [m/s] et non au Lag [s] à proprement parlé. C'est en ce sens que cela créer une confusion.
> Je pense donc qu'il y a deux solutions :
>
> 1) Garder la nomenclature Lag et donc demander à Run_ModelLag comme paramètres un vecteur de Lag en secondes (de la taille du nombre de bassins amonts).
> 2) Garder le paramétrage actuel, c'est à dire une valeur unique de vitesse d'écoulement, et simplement renommer "Lag" en "velocity", "flow_velocity", ou quelque chose dans le genre.
What do you think @david.dorchies ? I would say that we have to keep the parameter in m/s, but maybe its name is not clear and naming it velocity directly would be better?v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/100Unclear message when Qupstream is NA2021-03-24T12:00:30+01:00Thirel GuillaumeUnclear message when Qupstream is NAWhen we have NA in upstream discharge (observations), the error message is unclear.
For a run (I modified the following in the example `InputsModel$Qupstream[5000] <- NA`) :
```
OutputsModel <- RunModel_Lag(InputsModel = InputsModel,
...When we have NA in upstream discharge (observations), the error message is unclear.
For a run (I modified the following in the example `InputsModel$Qupstream[5000] <- NA`) :
```
OutputsModel <- RunModel_Lag(InputsModel = InputsModel,
+ RunOptions = RunOptions, Param = Lag)
Error in if (any(OutputsModel$Qsim < 0)) { :
valeur manquante là où TRUE / FALSE est requis
```
For a calibration (according to @cyril.thebault):
```
Grid-Screening in progress(0%Error in FUN_MOD(InputsModel, RunOptions = RunOptions, Param = Param[iFirstParamRunOffModel:length(Param)]):
NA/NaN/Inf in foreign function call (arg2)
```
@olivier.delaigue and @david.dorchies, I think we need to better check NAs. In my opinion, NAs should be forbidden, such as are NAs for P and T.v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/102RunModel_Lag returns 2 values for a one time step run2021-03-24T14:41:19+01:00Dorchies DavidRunModel_Lag returns 2 values for a one time step runThe example below tries to run a daily SD model for the time step `1990-01-01` (adapted from `tests\testthat\test-RunModel_Lag.R`):
```r
library(airGR)
data(L0123001)
BasinAreas <- c(BasinInfo$BasinArea, BasinInfo$BasinArea)
# Qupstream...The example below tries to run a daily SD model for the time step `1990-01-01` (adapted from `tests\testthat\test-RunModel_Lag.R`):
```r
library(airGR)
data(L0123001)
BasinAreas <- c(BasinInfo$BasinArea, BasinInfo$BasinArea)
# Qupstream = sinusoid synchronised on hydrological year from 0 mm to mean value of Qobs
Qupstream <- floor((sin((seq_along(BasinObs$Qmm)/365*2*3.14))+1) * mean(BasinObs$Qmm, na.rm = TRUE))
InputsModel <- CreateInputsModel(
FUN_MOD = RunModel_GR4J,
DatesR = BasinObs$DatesR,
Precip = BasinObs$P,
PotEvap = BasinObs$E,
Qupstream = matrix(Qupstream, ncol = 1),
LengthHydro = 1000,
BasinAreas = BasinAreas
)
Ind_Run <- seq(which(format(BasinObs$DatesR, format = "%Y-%m-%d") == "1990-01-01"),
which(format(BasinObs$DatesR, format = "%Y-%m-%d") == "1990-01-01"))
RunOptions <- suppressWarnings(CreateRunOptions(FUN_MOD = RunModel_GR4J,
InputsModel = InputsModel,
IndPeriod_Run = Ind_Run))
Param <- c(1., 257.237556, 1.012237, 88.234673, 2.207958)
OutputsModel <- RunModel(InputsModel = InputsModel, RunOptions = RunOptions, Param = Param, FUN_MOD = RunModel_GR4J)
OutputsModel$Qsim
```
The output of this code is:
```r
> OutputsModel$Qsim
[1] 1.709998 1.715785
```
The error comes from the lines in `R\RunModel_Lag.R`:
```r
OutputsModel$Qsim <- OutputsModel$Qsim +
c(IniStates[[upstream_basin]][-length(IniStates[[upstream_basin]])],
Qupstream[1:(LengthTs - floor(PT[upstream_basin]))]) *
HUTRANS[1, upstream_basin] +
c(IniStates[[upstream_basin]],
Qupstream[1:(LengthTs - floor(PT[upstream_basin]) - 1)]) *
HUTRANS[2, upstream_basin]
```
Especially from the expression `(LengthTs - floor(PT[upstream_basin]) - 1)` which is equal to zero when `LengthTs = 1`. Initial states and Qupstream should be merge only if we need to address them.
I tried locally to solve the problem with this solution:
```r
Qupstream1 <- c(IniStates[[upstream_basin]][-length(IniStates[[upstream_basin]])], Qupstream[1:(LengthTs - floor(PT[upstream_basin]))])
Qupstream2 <- IniStates[[upstream_basin]]
if(LengthTs - floor(PT[upstream_basin]) - 1 > 0) Qupstream2 <- c(Qupstream2, Qupstream[1:(LengthTs - floor(PT[upstream_basin]) - 1)])
OutputsModel$Qsim <- OutputsModel$Qsim +
Qupstream1 * HUTRANS[1, upstream_basin] +
Qupstream2 * HUTRANS[2, upstream_basin]
```v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/103RunModel_Lag: `StateEnd` is wrong in case of more than 1 upstream basin2021-03-24T14:41:19+01:00Dorchies DavidRunModel_Lag: `StateEnd` is wrong in case of more than 1 upstream basinThe code refers to the variable `Qupstream` which is the flow of the last upstream flow processed in the loop of lag calculation:
```r
OutputsModel$StateEnd$SD <- lapply(seq(NbUpBasins), function(x) {
Qupstream[(LengthTs - floor(...The code refers to the variable `Qupstream` which is the flow of the last upstream flow processed in the loop of lag calculation:
```r
OutputsModel$StateEnd$SD <- lapply(seq(NbUpBasins), function(x) {
Qupstream[(LengthTs - floor(PT[x])):LengthTs]
})
```
It should refer to `InputsModel$Qupstream` which is the matrix of all upstream flows.v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/101Fix vignette "Plugging in new calibration algorithms in airGR"2021-03-25T16:25:03+01:00François BourginFix vignette "Plugging in new calibration algorithms in airGR"The starting points used for the multi-start approach are in the real space, while they should be in the transformed space.
The line
```
startGR4J <- expand.grid(data.frame(CalibOptions$StartParamDistrib))
```
should be replaced by
`...The starting points used for the multi-start approach are in the real space, while they should be in the transformed space.
The line
```
startGR4J <- expand.grid(data.frame(CalibOptions$StartParamDistrib))
```
should be replaced by
```
StartParamDistrib <- matrix(c(+5.13, -1.60, +3.03, -9.05,
+5.51, -0.61, +3.74, -8.51,
+6.07, -0.02, +4.42, -8.06), ncol = 4, byrow = TRUE)
startGR4J <- expand.grid(data.frame(StartParamDistrib))
```
or perhaps, by a more diverse list of starting points sampled in the transformed space if we want to find local optima.v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/113Management of missing values in input and output of Fortran codes2021-03-27T07:28:34+01:00Delaigue OlivierManagement of missing values in input and output of Fortran codesAs written by @francois.bourgin, the following lines slow down a lot the `RunModel_*()` functions (especially if the chronicles are long). https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/98#note_34812
By removing the use of the `rou...As written by @francois.bourgin, the following lines slow down a lot the `RunModel_*()` functions (especially if the chronicles are long). https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/98#note_34812
By removing the use of the `round()` function, the functions arec speeded up. https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/98#note_34831
See the discussion on issue https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/98v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/109RunModel_Lag: Add an argument `QcontribDown` to `RunModel_Lag`2021-04-06T10:53:40+02:00Dorchies DavidRunModel_Lag: Add an argument `QcontribDown` to `RunModel_Lag`Currently, the runoff contribution of the downstream sub-basin is included in the `InputsModel`object by copying the `OutputsModel` object of the GR simulation (extracted from `RunModel_lag` example script):
```
# Add the output of the ...Currently, the runoff contribution of the downstream sub-basin is included in the `InputsModel`object by copying the `OutputsModel` object of the GR simulation (extracted from `RunModel_lag` example script):
```
# Add the output of the precipitation-runoff model in the InputsModel object
InputsModel$OutputsModel <- OutputsModelDown
# Run the lag model which routes precipitation-runoff model and upstream flows
OutputsModel <- RunModel_Lag(InputsModel = InputsModel,
RunOptions = RunOptions, Param = Velocity)
```
This way of proceeding is:
- ugly :sweat_smile:
- not generic if you want to inject another contribution than a GR model output
- cumbersome because we don't need to copy all informations contained in `OutputsModel`, only `Qsim`
Using a new argument `QcontribDown` in `RunModel_Lag` for the runoff contribution of the downstream sub-basin would be cleaner and more generic.v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/104RunModel_Lag: `StateEnd` is wrong if upstream flow is in mm / time step2021-04-06T15:01:00+02:00Dorchies DavidRunModel_Lag: `StateEnd` is wrong if upstream flow is in mm / time stepIn the lag model upstream flows related to a sub-basin are converted from mm / time step to m3 / time step. Initial states and upstream flows are merged in order to have the memory of the upstream flows before the simulation period but n...In the lag model upstream flows related to a sub-basin are converted from mm / time step to m3 / time step. Initial states and upstream flows are merged in order to have the memory of the upstream flows before the simulation period but no conversion is applied to initial states.
So the initial states should either be converted if they are stored in mm/ts or stored in m3/ts.
There are pro and cons for the 2 solutions:
1. store states in mm / ts and converted in simulation: `Qupstream` is already in mm/ts so it seems consistent to store the corresponding initial state (which is `Qupstream` before the simulation period) in the same unity.
2. store states in m3/ts and use it directly in simulation which can avoid a computation burden due to a conversion at each run.
Concerning the second solution, maybe it could be a good idea to also convert `Qupstream` in m3/ts in `CreateInputsModel` in order to avoid an "on-the-fly" conversion at each run of `RunModel_Lag`. So, all upstream flows (States + Qupstream) would be in m3/ts and no computation burden would occur due to conversion.v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/110SD model: add an argument `unit` in CreateInputsModel and store `Qupstream` i...2021-04-15T21:35:10+02:00Dorchies DavidSD model: add an argument `unit` in CreateInputsModel and store `Qupstream` in m3/time stepThe current implementation mixes units in `Qupstream` depending on the fact the upstream flow is related to an area or not and this leads to on-the-fly conversion in `RunModel_Lag` (See #104).
Moreover, the presence of different units i...The current implementation mixes units in `Qupstream` depending on the fact the upstream flow is related to an area or not and this leads to on-the-fly conversion in `RunModel_Lag` (See #104).
Moreover, the presence of different units in `Qupstream` can be confusing for the user.
Implementation of `Qupstream` in m3/time-step in the `InputsModel` object would avoid unnecessary conversions during simulation but upstream flow provided by GR models are in mm/time-step, so giving the choice of the unit to the user appears to be a good trade-off.
Here a proposition of the `unit` parameter definition:
```
\item{unit}{(optional) [character] unit of the flow in the parameter \code{Qupstream}, available units are: "mm" for mm/time-step (default), "m3" for m3/time-step, "m3/s" and "l/s". See details}
\item{Qupstream}{(optional) [numerical matrix] time series of upstream flows (catchment average), its unit is defined by the \code{unit} parameter, required to create the SD model inputs. See details}
(...)
\details{
(...)
Users wanting to use a semi-distributed (SD) lag model should provide valid \code{Qupstream}, \code{LengthHydro}, and \code{BasinAreas} parameters. Each upstream sub-catchment is described by an upstream flow time series (one column in \code{Qupstream} matrix), a distance between the downstream outlet and the upstream inlet (one item in \code{LengthHydro}) and an area (one item in \code{BasinAreas}).
The order of the columns or of the items should be consistent for all these parameters. \code{BasinAreas} should contain one extra information (stored in the last item) which is the area of the downstream sub-catchment.
Upstream flows that are not related to a sub-catchment such as release or withdraw spots are identified by an area equal to \code{NA}, and if \code{unit="mm"} the upstream flow must be expressed in m3/time step instead of mm/time step which is not possible in absence of a related area.
Please note that the use of SD lag model require to use the \code{\link{RunModel}} function instead of \code{\link{RunModel_GR4J}} or the other \code{RunModel_*} functions.
```v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/108RunModel_Lag crashes when called from RunModel2021-04-18T07:52:00+02:00Dorchies DavidRunModel_Lag crashes when called from RunModelTry this:
```
library(airGR)
example(RunModel_Lag)
OutputsModel <- RunModel(InputsModel = InputsModel,RunOptions = RunOptions, Param = Velocity, FUN_MOD = RunModel_Lag)
```
The following error occurs:
`Error in rep(0, floor(PT[x] + 1))...Try this:
```
library(airGR)
example(RunModel_Lag)
OutputsModel <- RunModel(InputsModel = InputsModel,RunOptions = RunOptions, Param = Velocity, FUN_MOD = RunModel_Lag)
```
The following error occurs:
`Error in rep(0, floor(PT[x] + 1)) : invalid 'times' argument`
@olivier.delaigue and @guillaume.thirel, despite what we discussed, I think it looks like a normal way of using the model as we use the other ones.v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/114Add explanations linked to semi-distributed model2021-04-20T21:06:20+02:00Delaigue OlivierAdd explanations linked to semi-distributed modelClearly write that the models can be used in a semi-distributed mode in the following files:
- DECRIPTION file ("description" section)
- airGR.rd file (with a link to the "V05_sd_model" vignette)
- README file (with a link to the "V05_s...Clearly write that the models can be used in a semi-distributed mode in the following files:
- DECRIPTION file ("description" section)
- airGR.rd file (with a link to the "V05_sd_model" vignette)
- README file (with a link to the "V05_sd_model" vignette)v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/90Fix incorrect parameter transformation for X5 in GR5J2021-04-20T21:26:56+02:00Pelletier AntoineFix incorrect parameter transformation for X5 in GR5JIn `TransfoParam_GR5J.R`, the proposed transformation for X5 only allows values between 0 and 1. Indeed, with `ParamOut[, 5] <- (ParamIn[, 5] + 9.99) / 19.98`, with `ParamIn[, 5]` between -9.99 and 9.99, we get bounds of 0 and 1.
Yet, X...In `TransfoParam_GR5J.R`, the proposed transformation for X5 only allows values between 0 and 1. Indeed, with `ParamOut[, 5] <- (ParamIn[, 5] + 9.99) / 19.98`, with `ParamIn[, 5]` between -9.99 and 9.99, we get bounds of 0 and 1.
Yet, X5 can take values below 0 and above 1 (see figure 8 of [this article](https://doi.org/10.1016/j.jhydrol.2011.09.034) for instance). The proposed transformation in `TransfoParam_GR6J.R`, which is `ParamOut[, 5] <- ParamIn[, 5] / 5`, seem to be more consistent with the expected values of the parameter, in addition to improving coherence between the two versions of the model.v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/106RunModel_CemaNeige fails in CreateInputsModel at the hourly time step2021-04-23T10:50:55+02:00François BourginRunModel_CemaNeige fails in CreateInputsModel at the hourly time stepThe description of `RunModel_CemaNeige {airGR}` states:
> Function which performs a single run for the CemaNeige snow module at the daily or hourly time step.
Unfortunately, `CreateInputsModel` does not work at the hourly time step beca...The description of `RunModel_CemaNeige {airGR}` states:
> Function which performs a single run for the CemaNeige snow module at the daily or hourly time step.
Unfortunately, `CreateInputsModel` does not work at the hourly time step because of the following lines:
```
if (identical(FUN_MOD, RunModel_CemaNeige)) {
ObjectClass <- c(ObjectClass, "daily", "CemaNeige")
TimeStep <- as.integer(24 * 60 * 60)
BOOL <- TRUE
}
```v1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/50Adjust TransfoParam_GR5H (production store)2021-04-23T12:11:01+02:00Thirel GuillaumeAdjust TransfoParam_GR5H (production store)The transformation given for GR5H (coming from Andrea Ficchi's files) is different from other models regarding the production store.
`ParamOut[, 1] <- exp(1.5 * ParamIn[, 1])` vs `ParamOut[, 1] <- exp(ParamIn[, 1])` and `ParamOut[, 1] <...The transformation given for GR5H (coming from Andrea Ficchi's files) is different from other models regarding the production store.
`ParamOut[, 1] <- exp(1.5 * ParamIn[, 1])` vs `ParamOut[, 1] <- exp(ParamIn[, 1])` and `ParamOut[, 1] <- log(ParamIn[, 1])/1.5` vs `ParamOut[, 1] <- log(ParamIn[, 1])`.
The 1.5 factor seems unnecessary as it provokes irrealistically high X1 values. We should try to remove it and check the impact on GR5H on our 240 catchments hourly dataset.
Issue made following a remark from @paul.astagneauv1.6.11https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/86Gitlab-CI: correctly handle WARNING and NOTE in checks2021-06-22T08:53:02+02:00Dorchies DavidGitlab-CI: correctly handle WARNING and NOTE in checksCurrently, check fails only if in an ERROR occurs especially because some warnings or notes occur because of reasons outside the scope of the package itself (dependency to 'qpdf' or curl issues like in https://gitlab.irstea.fr/HYCAR-Hydr...Currently, check fails only if in an ERROR occurs especially because some warnings or notes occur because of reasons outside the scope of the package itself (dependency to 'qpdf' or curl issues like in https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/jobs/142218).
airGRiwrm now implements a new CI procedure that correcly handle WARNING in check by putting the job and the pipeline in failure state (See: https://gitlab.irstea.fr/in-wop/airGRiwrm/-/commits/master/.gitlab-ci.yml). This way of doing seems reasonable because of the CRAN's requirements which doesn't allow any WARNING in the check of a package submission.
This procedure can be implemented in airGR, especially the part using R tidyverse docker image hosted on the server hosted at INRAE Lyon.
A new concept "don't stop on failure" exists in Gitlab-CI (See: https://docs.gitlab.com/ee/ci/yaml/#allow_failure) that allows to continue a pipeline on a job failure and to display corresponding job and pipeline in an "orange warning state". That could be useful if we want to be aware of NOTEs in checks without setting all the check process in a red failure state.v1.6.11Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/112Change LengthHydro unit2021-07-14T09:12:31+02:00Delaigue OlivierChange LengthHydro unitAfter discussion, it seems that it would be better to change the unit of `LengthHydro` from [m] to [km].
In this case, it is necessary to modify:
- code of `RunModel_Lag()`
- man of `CreateInputsModel()`
- man (example) of `RunModel_La...After discussion, it seems that it would be better to change the unit of `LengthHydro` from [m] to [km].
In this case, it is necessary to modify:
- code of `RunModel_Lag()`
- man of `CreateInputsModel()`
- man (example) of `RunModel_Lag()`
- vignette "sd_model"
- some testsv1.6.11Dorchies DavidDorchies David