airGR issueshttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues2021-03-02T17:57:29+01:00https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/104RunModel_Lag: `StateEnd` is wrong if upstream flow is in mm / time step2021-03-02T17:57:29+01: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 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.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.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/103RunModel_Lag: `StateEnd` is wrong in case of more than 1 upstream basin2021-03-08T08:33:26+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(PT[x])):LengthTs]
})
```
It should refer to `InputsModel$Qupstream` which is the matrix of all upstream flows.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.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/102RunModel_Lag returns 2 values for a one time step run2021-03-02T17:57:29+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 = 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]
```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.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/101Fix vignette "Plugging in new calibration algorithms in airGR"2021-03-02T08:41:35+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
```
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.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.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/100Unclear message when Qupstream is NA2021-03-02T08:41:14+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,
+ 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.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.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/98Calculate computation times for each CRAN version2021-03-02T08:41:49+01:00Thirel GuillaumeCalculate computation times for each CRAN versionIn order to keep trace of the evolution of the computing times after modifications of the packages, I think we should automatically calculate the computation times in a similar way as done in Coron et al. (2017) (https://doi.org/10.1016/j.envsoft.2017.05.002), Table B.3.
I propose that:
- each former CRAN version is tested now
- each future new version of the package is tested (before submission for detecting problems(?) and after publication on the CRAN)
- all tests done in Tab. B.3 are performed (i.e. runs and calibrations over 10 years for each hydrological + snow model combinations, 100 tries)
- other functions like `Imax`, `PE_Oudin`, `DataAltiExtrapolation_Valery` are tested (configuration to determine)
- at least one Windows machine and one Linux machine are tested, Mac if possible.In order to keep trace of the evolution of the computing times after modifications of the packages, I think we should automatically calculate the computation times in a similar way as done in Coron et al. (2017) (https://doi.org/10.1016/j.envsoft.2017.05.002), Table B.3.
I propose that:
- each former CRAN version is tested now
- each future new version of the package is tested (before submission for detecting problems(?) and after publication on the CRAN)
- all tests done in Tab. B.3 are performed (i.e. runs and calibrations over 10 years for each hydrological + snow model combinations, 100 tries)
- other functions like `Imax`, `PE_Oudin`, `DataAltiExtrapolation_Valery` are tested (configuration to determine)
- at least one Windows machine and one Linux machine are tested, Mac if possible.v1.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/97Parallelize CemaNeige2021-03-02T09:59:34+01:00Thirel GuillaumeParallelize CemaNeigeSimilarly to https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/96, the computation done on several altitude bands of CemaNeige could be parallelized. This remark is potentially valid both for `RunModel_CemaNeige*` calculation but also for `DataAltiExtrapolation_Valery`.Similarly to https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/96, the computation done on several altitude bands of CemaNeige could be parallelized. This remark is potentially valid both for `RunModel_CemaNeige*` calculation but also for `DataAltiExtrapolation_Valery`.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/96Parallelize the grid-screening step during calibration2021-03-02T08:42:04+01:00Delaigue OlivierParallelize the grid-screening step during calibrationIt is possible to parallelize the grid-screening step during calibration, this would speed up the calibration when the model used has many parameters, especially for some 'airGRplus' models.
We can use the 'parallel' package in order not to depend to an external package, because this one is automatically installed with R.It is possible to parallelize the grid-screening step during calibration, this would speed up the calibration when the model used has many parameters, especially for some 'airGRplus' models.
We can use the 'parallel' package in order not to depend to an external package, because this one is automatically installed with R.v1.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/91Remove plot.OutputsModel from NAMESPACE2021-01-15T08:47:47+01:00Delaigue OlivierRemove plot.OutputsModel from NAMESPACEThe `plot.OutputsModel()` function has been export again in order not to break the reverse dependency of the current version of airGRteaching (v0.2.9.25) available on the CRAN (see #89, ab853bd3). This break has already been fixed in the development version of airGRteaching ([v0.2.10](https://gitlab.irstea.fr/HYCAR-Hydro/airgrteaching/-/milestones/1)) that will be submitted to CRAN shortly after airGR [v1.6](https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/milestones/2).
Once the new versions of airGR and airGRteaching will be available on CRAN. This function can be removed from the NAMESPACE again from the development version of airGR.The `plot.OutputsModel()` function has been export again in order not to break the reverse dependency of the current version of airGRteaching (v0.2.9.25) available on the CRAN (see #89, ab853bd3). This break has already been fixed in the development version of airGRteaching ([v0.2.10](https://gitlab.irstea.fr/HYCAR-Hydro/airgrteaching/-/milestones/1)) that will be submitted to CRAN shortly after airGR [v1.6](https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/milestones/2).
Once the new versions of airGR and airGRteaching will be available on CRAN. This function can be removed from the NAMESPACE again from the development version of airGR.v2.0https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/90Incorrect parameter transformation for X5 in GR5J2021-01-29T11:24:20+01:00Pelletier AntoineIncorrect 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, 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.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.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/86Gitlab-CI: correctly handle WARNING and NOTE in checks2021-01-15T16:38:51+01: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-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.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.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/84Add the use of '['.OutputsModel in plot.OutputsModel2021-01-11T18:10:13+01:00Delaigue OlivierAdd the use of '['.OutputsModel in plot.OutputsModelIt could be nice to simplify the code of the `plot.OutputsModel()` function by the use of '['.OutputsModel in order to manage the `IndPeriod_Plot` argument.It could be nice to simplify the code of the `plot.OutputsModel()` function by the use of '['.OutputsModel in order to manage the `IndPeriod_Plot` argument.v2.0https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/81Remove of the oldest deprecated arguments and functions2021-01-11T18:14:20+01:00Delaigue OlivierRemove of the oldest deprecated arguments and functionsCheck the box when the argument or function is removed.
### 1.0.9.64 Release Notes (2017-11-10)
- [x] `RunSnowModule` argument is now deprecated in `CreateRunOptions()`.
### 1.0.14.1 Release Notes (2018-09-28)
- [ ] `LatRad` argument is now deprecated in `PEdaily_Oudin()` and replaced by the `Lat` argument.
- [ ] unused `Ind_zeroes` argument of the `CreateInputsCrit()` function is now deprecated.
- [ ] `verbose` argument is now deprecated in `CreateInputsCrit()` and replaced by the `warnings` argument.
### 1.2.13.16 Release Notes (2019-04-03)
- [ ] `Qobs` argument is now deprecated in `CreateInputsCrit()` and has been renamed `Obs`.
- [ ] `FUN_CRIT` argument is now deprecated in `ErrorCrit()`. This function now gets this information from the `InputsCrit` argument.
- [ ] `FUN_CRIT` argument is now deprecated in `Calibration_Michel()`. This function now gets this information from the `InputsCrit` argument.
### 1.3.2.23 Release Notes (2019-06-20)
- [ ] `PEdaily_Oudin()` function is deprecated and his use has been replaced by the use of `PE_Oudin()`.
### 1.6.8.44 Release Notes (2021-01-08)
- [ ] `TimeFormat` argument is now deprecated in `SeriesAggreg()`.
- [ ] `NewTimeFormat` argument is now deprecated in `SeriesAggreg()` and replaced by the `Format` argument.Check the box when the argument or function is removed.
### 1.0.9.64 Release Notes (2017-11-10)
- [x] `RunSnowModule` argument is now deprecated in `CreateRunOptions()`.
### 1.0.14.1 Release Notes (2018-09-28)
- [ ] `LatRad` argument is now deprecated in `PEdaily_Oudin()` and replaced by the `Lat` argument.
- [ ] unused `Ind_zeroes` argument of the `CreateInputsCrit()` function is now deprecated.
- [ ] `verbose` argument is now deprecated in `CreateInputsCrit()` and replaced by the `warnings` argument.
### 1.2.13.16 Release Notes (2019-04-03)
- [ ] `Qobs` argument is now deprecated in `CreateInputsCrit()` and has been renamed `Obs`.
- [ ] `FUN_CRIT` argument is now deprecated in `ErrorCrit()`. This function now gets this information from the `InputsCrit` argument.
- [ ] `FUN_CRIT` argument is now deprecated in `Calibration_Michel()`. This function now gets this information from the `InputsCrit` argument.
### 1.3.2.23 Release Notes (2019-06-20)
- [ ] `PEdaily_Oudin()` function is deprecated and his use has been replaced by the use of `PE_Oudin()`.
### 1.6.8.44 Release Notes (2021-01-08)
- [ ] `TimeFormat` argument is now deprecated in `SeriesAggreg()`.
- [ ] `NewTimeFormat` argument is now deprecated in `SeriesAggreg()` and replaced by the `Format` argument.v2.0https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/72Add a CONTRIBUTING.md file to the repository2021-01-28T09:28:06+01:00Dorchies DavidAdd a CONTRIBUTING.md file to the repositoryAs several developers work on airGR, it would be a good idea to write development directives in an unified file.
Open source projects use a file named `CONTRIBUTING.md` which include these useful information.
For details about the content of such a file, please have a look to: https://mozillascience.github.io/working-open-workshop/contributing/
It would be great to write some important information such as:
* the workflow: the reporter create a ticket, the developper create a branch and a merge request and work on this branch, the manager merge the branch when the job is done
* the syntax of the commit comments and the policy for versioning with how to fill the files `DESCRIPTION` and `NEWS.md`
* the code syntax: "Capital CamelCase" for variables and functions, 2 spaces tabs, max line length, brackets on `if` statements...
* the testing environment: how to test the package locally and how it is tested by Gitlab-CI
* the release procedure: how to publish the package on CRAN?As several developers work on airGR, it would be a good idea to write development directives in an unified file.
Open source projects use a file named `CONTRIBUTING.md` which include these useful information.
For details about the content of such a file, please have a look to: https://mozillascience.github.io/working-open-workshop/contributing/
It would be great to write some important information such as:
* the workflow: the reporter create a ticket, the developper create a branch and a merge request and work on this branch, the manager merge the branch when the job is done
* the syntax of the commit comments and the policy for versioning with how to fill the files `DESCRIPTION` and `NEWS.md`
* the code syntax: "Capital CamelCase" for variables and functions, 2 spaces tabs, max line length, brackets on `if` statements...
* the testing environment: how to test the package locally and how it is tested by Gitlab-CI
* the release procedure: how to publish the package on CRAN?https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/60Migrate TransfoParam functions and CreateCalibOptions to S3 methods2020-10-05T12:10:02+02:00Dorchies DavidMigrate TransfoParam functions and CreateCalibOptions to S3 methodsThe main idea is to facilitate the model chaining (e.g. CemaNeige + GR4J + LAG) in order to reduce the current code complexity and facilitate extension to new models in the future. S3 class concepts are a good candidate to tackle this issue. A Proof of Concept written in Rmarkdown showing how model chaining can be operated is available here :
[PoC_airGR_S3_classes.Rmd](/uploads/4492a10c93c3c7ba4547c76bf6f19896/PoC_airGR_S3_classes.Rmd)
## Starting point
Currently in `master` and `dev` branches, depending on the `FUN_MOD` provided to `CreateCalibOptions` a serie of conditions assign a `FUN1` variable corresponding to a `TransfoParam_GR*` function and a `FUN2` variable corresponding to either `TransfoParam_CemaNeige` or `TransfoParam_CemaNeigeHyst` depending on `isHyst` parameter. If several models are involved (e.g. CemaNeige + GR4J), then a "meta" FUN_TRANSFO function is written, binding columns of the output matrix with outputs of each model. In the `SD` branch the complexity of the process is again complicated by adding parameters of the LAG model.
## Proposition
- Modify CreateCalibOption in order to assign classes corresponding to the models involved in the model chain to the and call a generic function `TransfoParam`
- Change the name of `transfoParam_*` function to `TransfoParam.*` methods
- Rewrite `TransfoParam` generic function in order to automatically bind columns of the matrix `ParamT`
## Pitfalls
If we assume a generic form of the model chaining and put in correspondence the order of the model with the order of the parameters, the current order in `SD` implementation is not compliant: the current order is : `param = c(GRparam, CemaneigeParam, LAGparam)`. If we considere `Cemaneige` as a pretreatment of the rain, before running a `GR` model and `LAG` a routing model applied after GR model, this order of parameter is not logical.
To solve this issue, as the order of parameter between `GR` and `Cemaneige` is already fixed on older versions and SD model is not public yet, I propose to adopt this order of parameters: `param = c(LAGparam, GRparam, CemaneigeParam)`.The main idea is to facilitate the model chaining (e.g. CemaNeige + GR4J + LAG) in order to reduce the current code complexity and facilitate extension to new models in the future. S3 class concepts are a good candidate to tackle this issue. A Proof of Concept written in Rmarkdown showing how model chaining can be operated is available here :
[PoC_airGR_S3_classes.Rmd](/uploads/4492a10c93c3c7ba4547c76bf6f19896/PoC_airGR_S3_classes.Rmd)
## Starting point
Currently in `master` and `dev` branches, depending on the `FUN_MOD` provided to `CreateCalibOptions` a serie of conditions assign a `FUN1` variable corresponding to a `TransfoParam_GR*` function and a `FUN2` variable corresponding to either `TransfoParam_CemaNeige` or `TransfoParam_CemaNeigeHyst` depending on `isHyst` parameter. If several models are involved (e.g. CemaNeige + GR4J), then a "meta" FUN_TRANSFO function is written, binding columns of the output matrix with outputs of each model. In the `SD` branch the complexity of the process is again complicated by adding parameters of the LAG model.
## Proposition
- Modify CreateCalibOption in order to assign classes corresponding to the models involved in the model chain to the and call a generic function `TransfoParam`
- Change the name of `transfoParam_*` function to `TransfoParam.*` methods
- Rewrite `TransfoParam` generic function in order to automatically bind columns of the matrix `ParamT`
## Pitfalls
If we assume a generic form of the model chaining and put in correspondence the order of the model with the order of the parameters, the current order in `SD` implementation is not compliant: the current order is : `param = c(GRparam, CemaneigeParam, LAGparam)`. If we considere `Cemaneige` as a pretreatment of the rain, before running a `GR` model and `LAG` a routing model applied after GR model, this order of parameter is not logical.
To solve this issue, as the order of parameter between `GR` and `Cemaneige` is already fixed on older versions and SD model is not public yet, I propose to adopt this order of parameters: `param = c(LAGparam, GRparam, CemaneigeParam)`.v2.0Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/58T gradient in DataAltiExtrapolation_Valery only valid in Northern hemisphere2021-01-11T12:02:48+01:00Thirel GuillaumeT gradient in DataAltiExtrapolation_Valery only valid in Northern hemisphereI suspect that the DataAltiExtrapolation_Valery T gradient defined in GradT_Valery2010() is only valid for the Northern hemisphere.I suspect that the DataAltiExtrapolation_Valery T gradient defined in GradT_Valery2010() is only valid for the Northern hemisphere.v2.0https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/57Number of IniStates and StateEnd values when only CemaNeige is used2020-05-26T14:13:32+02:00Thirel GuillaumeNumber of IniStates and StateEnd values when only CemaNeige is usedFor initial states (e.g. `RunOptions$IniStates`) or final states (`OutputsModel$StateEnd`, we store `7 + 60(*24) + Nlayers*4` states at the daily or hourly time steps. This is not useful when only `CemaNeige `is used. For exemple, for `GR2M `we store only `2` states, not `7 + 60`.
I propose to modify `CreateRunOptions `lines accordingly:
```
NState <- NULL
if ("GR" %in% ObjectClass | "CemaNeige" %in% ObjectClass) {
if ("hourly" %in% ObjectClass) {
NState <- 7 + 3 * 24 * 20+ 4 * NLayers
}
if ("daily" %in% ObjectClass) {
NState <- 7 + 3 * 20 + 4 * NLayers
}
if ("monthly" %in% ObjectClass) {
NState <- 2
}
if ("yearly" %in% ObjectClass) {
NState <- 1
}
}
```
should become :
```
NState <- 0
if ("GR" %in% ObjectClass) {
if ("hourly" %in% ObjectClass) {
NState <- 7 + 3 * 24 * 20
}
if ("daily" %in% ObjectClass) {
NState <- 7 + 3 * 20
}
if ("monthly" %in% ObjectClass) {
NState <- 2
}
if ("yearly" %in% ObjectClass) {
NState <- 1
}
}
if ("CemaNeige" %in% ObjectClass) {
if (!"yearly" %in% ObjectClass) {
NState <- NState + 4 * NLayers
}
}
```
In addition, `RunModel_CemaNeige.R` and `CreateIniStates.R` should be modified accordingly. This will ease the implementation of `RunModel_CemaNeigeGR2M`.For initial states (e.g. `RunOptions$IniStates`) or final states (`OutputsModel$StateEnd`, we store `7 + 60(*24) + Nlayers*4` states at the daily or hourly time steps. This is not useful when only `CemaNeige `is used. For exemple, for `GR2M `we store only `2` states, not `7 + 60`.
I propose to modify `CreateRunOptions `lines accordingly:
```
NState <- NULL
if ("GR" %in% ObjectClass | "CemaNeige" %in% ObjectClass) {
if ("hourly" %in% ObjectClass) {
NState <- 7 + 3 * 24 * 20+ 4 * NLayers
}
if ("daily" %in% ObjectClass) {
NState <- 7 + 3 * 20 + 4 * NLayers
}
if ("monthly" %in% ObjectClass) {
NState <- 2
}
if ("yearly" %in% ObjectClass) {
NState <- 1
}
}
```
should become :
```
NState <- 0
if ("GR" %in% ObjectClass) {
if ("hourly" %in% ObjectClass) {
NState <- 7 + 3 * 24 * 20
}
if ("daily" %in% ObjectClass) {
NState <- 7 + 3 * 20
}
if ("monthly" %in% ObjectClass) {
NState <- 2
}
if ("yearly" %in% ObjectClass) {
NState <- 1
}
}
if ("CemaNeige" %in% ObjectClass) {
if (!"yearly" %in% ObjectClass) {
NState <- NState + 4 * NLayers
}
}
```
In addition, `RunModel_CemaNeige.R` and `CreateIniStates.R` should be modified accordingly. This will ease the implementation of `RunModel_CemaNeigeGR2M`.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/56Remove useless time step checks in the plot.OutputsModel function2020-11-20T11:36:26+01:00Delaigue OlivierRemove useless time step checks in the plot.OutputsModel functionThe time step of the `OutputsModel` argument is checked by the `plot.OutputsModel()` function (the time step is checked by comparing with the calculation of the difference of the last two time steps). This seems useless, because we already check the class of this object, which is therefore assumed to be necessarily valid.
In addition, this stops the function from being used on an aggregated series returned by the `SeriesAggreg2.OutputsModel ()` function.The time step of the `OutputsModel` argument is checked by the `plot.OutputsModel()` function (the time step is checked by comparing with the calculation of the difference of the last two time steps). This seems useless, because we already check the class of this object, which is therefore assumed to be necessarily valid.
In addition, this stops the function from being used on an aggregated series returned by the `SeriesAggreg2.OutputsModel ()` function.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/54Include a dam module with the lumped GR models2020-04-23T17:22:27+02:00Thirel GuillaumeInclude a dam module with the lumped GR modelsFollowing the PhD thesis of Jean-Luc Payan and the work of Morgane Terrier, we have pieces of `airGR `codes that allow to take into account the impact of dams in the GR4J and GR5J models. Knowing a time series of delta V, i.e. the variation of the volume of water in a dam, it is possible to subtract water from the production store and to release in the the routing store.
Not sure about GR6J, Morgane told me it caused issues with the exponential store, to be checked with @charles.perrin .
Eventual inclusion of this module in `airGR `should be thought of considering parallel works on semi-distribution.
Also important will be how to deal with this additional input data (observed delta V time series).Following the PhD thesis of Jean-Luc Payan and the work of Morgane Terrier, we have pieces of `airGR `codes that allow to take into account the impact of dams in the GR4J and GR5J models. Knowing a time series of delta V, i.e. the variation of the volume of water in a dam, it is possible to subtract water from the production store and to release in the the routing store.
Not sure about GR6J, Morgane told me it caused issues with the exponential store, to be checked with @charles.perrin .
Eventual inclusion of this module in `airGR `should be thought of considering parallel works on semi-distribution.
Also important will be how to deal with this additional input data (observed delta V time series).https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/50Adjustment of TransfoParam_GR5H (production store)2021-03-05T09:18:44+01:00Thirel GuillaumeAdjustment of 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] <- 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.astagneau 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.astagneau