airGR issueshttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues2021-03-19T09:59:24+01:00https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/111Regularization calibration2021-03-19T09:59:24+01:00Thirel GuillaumeRegularization calibrationIt would be nice to implement the regularisation calibration proposed by de Lavenne et al. (2019) (https://doi.org/10.1029/2018WR024266). That would stabilise parameters for downstream basins.
Basically, we need to consider in the calibration process the distance between calibrated parameter values and parameter values of upstream catchments.
That would be useful for @cyril.thebault as well as Laura and other people using airGR-SD.
I would say that we need for that:
- to have access to upstream parameter values in `Calibration_Michel`
- add a `regul` boolean in CreateCalibOptions and retrieve it in `Calibration_Michel`
- either a new `Error_Crit` that allows to combine any `Error_Crit` with the distance calculation, or implement directly that in `Calibration_Michel`
- distance must be calculated in the transformed space of parameters to avoid having parameters with huge distance (e.g. X1) compared to smaller parameters (e.g. X4)
- certainly other things that will come up when we try to implement it. :)
Who wants to give it a try? :)It would be nice to implement the regularisation calibration proposed by de Lavenne et al. (2019) (https://doi.org/10.1029/2018WR024266). That would stabilise parameters for downstream basins.
Basically, we need to consider in the calibration process the distance between calibrated parameter values and parameter values of upstream catchments.
That would be useful for @cyril.thebault as well as Laura and other people using airGR-SD.
I would say that we need for that:
- to have access to upstream parameter values in `Calibration_Michel`
- add a `regul` boolean in CreateCalibOptions and retrieve it in `Calibration_Michel`
- either a new `Error_Crit` that allows to combine any `Error_Crit` with the distance calculation, or implement directly that in `Calibration_Michel`
- distance must be calculated in the transformed space of parameters to avoid having parameters with huge distance (e.g. X1) compared to smaller parameters (e.g. X4)
- certainly other things that will come up when we try to implement it. :)
Who wants to give it a try? :)https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/110SD model: add an argument `unit` in CreateInputsModel and store `Qupstream` i...2021-04-08T16:21:36+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 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.
```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.7Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/108RunModel_Lag crashes when called from RunModel2021-04-07T12:12:43+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)) : 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.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.7Dorchies DavidDorchies Davidhttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/106RunModel_CemaNeige fails in CreateInputsModel at the hourly time step2021-03-29T14:18:59+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 because of the following lines:
```
if (identical(FUN_MOD, RunModel_CemaNeige)) {
ObjectClass <- c(ObjectClass, "daily", "CemaNeige")
TimeStep <- as.integer(24 * 60 * 60)
BOOL <- TRUE
}
```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.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/98Calculate computation times for each CRAN version2021-03-27T07:28:33+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-27T08:07:00+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-04-07T12:05:58+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-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.v1.7Dorchies DavidDorchies Davidhttps://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 methods2021-03-24T09:11:35+01: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-23T16:56:49+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.astagneauThe 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.7https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/48Computation time of the Imax function2020-04-15T08:07:25+02:00Delaigue OlivierComputation time of the Imax functionComputation time of the `Imax()` function needs to be improved.The use of the double loop slows down the code strongly.Computation time of the `Imax()` function needs to be improved.The use of the double loop slows down the code strongly.