airGR issueshttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues2020-10-05T12:10:02+02:00https://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/64Add precision in PE_Oudin doc2020-10-21T15:46:43+02:00Thirel GuillaumeAdd precision in PE_Oudin docThe Oudin paper used meteorological data at the station level to calculate PE. This is not specified but it is likely that point PE data were then basin averaged. (this is also what we do in our team when we use SAFRAN gridded data)
We should therefore add an advice to do that in the Details of the `PE_Oudin` doc. I propose the following:
> To calculate basin-wide Oudin potential evapotranspiration, it is advised, when possible, to use either station temperature or gridded temperature data to calculate PE and then average these PE values at the basin scale.The Oudin paper used meteorological data at the station level to calculate PE. This is not specified but it is likely that point PE data were then basin averaged. (this is also what we do in our team when we use SAFRAN gridded data)
We should therefore add an advice to do that in the Details of the `PE_Oudin` doc. I propose the following:
> To calculate basin-wide Oudin potential evapotranspiration, it is advised, when possible, to use either station temperature or gridded temperature data to calculate PE and then average these PE values at the basin scale.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/62Embed Oudin's Fortran code2020-10-21T15:07:19+02:00Delaigue OlivierEmbed Oudin's Fortran codeThe [PREMHYCE](https://gitlab.irstea.fr/HYCAR-Hydro/PREMHYCE) project need a faster version of the `PE_Oudin()` function.
The vectorization of the code is not satisfactory, it would be preferable to integrate the Fortran version of the code. I think that is a good idea to keep the R version of the code to facilitate the comprehension by the users. In this case we can add an argument to switch from the R code to the Fortran code.The [PREMHYCE](https://gitlab.irstea.fr/HYCAR-Hydro/PREMHYCE) project need a faster version of the `PE_Oudin()` function.
The vectorization of the code is not satisfactory, it would be preferable to integrate the Fortran version of the code. I think that is a good idea to keep the R version of the code to facilitate the comprehension by the users. In this case we can add an argument to switch from the R code to the Fortran code.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/61Adding the use of caRamel in the optimization vignette2020-06-29T09:57:48+02:00Delaigue OlivierAdding the use of caRamel in the optimization vignetteWe may add the use of the [**caRamel**](https://CRAN.R-project.org/package=caRamel) package in the optimization vignette (and the airGR vignette). A code is provided with [this article](https://www.hydrol-earth-syst-sci.net/24/3189/2020/hess-24-3189-2020.html). @guillaume.thirel provided an initial version of this piece of code in his review of the paper.We may add the use of the [**caRamel**](https://CRAN.R-project.org/package=caRamel) package in the optimization vignette (and the airGR vignette). A code is provided with [this article](https://www.hydrol-earth-syst-sci.net/24/3189/2020/hess-24-3189-2020.html). @guillaume.thirel provided an initial version of this piece of code in his review of the paper.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/58T gradient in DataAltiExtrapolation_Valery only valid in Northern hemisphere2020-05-15T13:36:51+02: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.https://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-05-01T09:54:20+02: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)2020-04-20T14:55:48+02: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 https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/49Add GR5H diagram in the RunModel_GR5H function help2020-04-15T17:06:30+02:00Thirel GuillaumeAdd GR5H diagram in the RunModel_GR5H function helpWe need to add the GR5H diagram in the `RunModel_GR5H `function help.We need to add the GR5H diagram in the `RunModel_GR5H `function help.https://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.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/46Add vignette about airGRmaps2020-04-10T17:19:55+02:00Delaigue OlivierAdd vignette about airGRmapsIt is probably interesting to write a vignette about the parameter maps of the GR4J, GR5J and GR6J models. These maps are available on the [airGRmaps](https://sunshine.irstea.fr/app/airGRmaps) shiny app. May be it could be mixed in the vignette whose title is* Generalist parameter sets for the GR4J model*.It is probably interesting to write a vignette about the parameter maps of the GR4J, GR5J and GR6J models. These maps are available on the [airGRmaps](https://sunshine.irstea.fr/app/airGRmaps) shiny app. May be it could be mixed in the vignette whose title is* Generalist parameter sets for the GR4J model*.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/45Deprecate verbose and warning arguments2020-04-10T18:16:58+02:00Delaigue OlivierDeprecate verbose and warning arguments@guillaume.thirel, are `warning` and `verbose` arguments really useful?
The use of the `suppresseMessages()` and `suppressWarnings()` functions allows you to do the same thing.
Furthermore, in some functions the argument is named `verbose`, but it is used to suppress warning messages (e.g. `SeriesAggreg()`)...@guillaume.thirel, are `warning` and `verbose` arguments really useful?
The use of the `suppresseMessages()` and `suppressWarnings()` functions allows you to do the same thing.
Furthermore, in some functions the argument is named `verbose`, but it is used to suppress warning messages (e.g. `SeriesAggreg()`)...https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/44Add an example using the BoolCrit argument of CreateInputsCrit2020-04-09T10:30:51+02:00Delaigue OlivierAdd an example using the BoolCrit argument of CreateInputsCritAdd an example using the `BoolCrit` argument of the `CreateInputsCrit()` function could be a good idea.
The difference with the `IndPeriod_Run` argument of the `CreateRunOptions()` function is not very intuitive for users.
This is particularly interesting for calibration above or below a flow threshold, for certain months of the year only, etc.
We should put this example in a help page or a vignette.Add an example using the `BoolCrit` argument of the `CreateInputsCrit()` function could be a good idea.
The difference with the `IndPeriod_Run` argument of the `CreateRunOptions()` function is not very intuitive for users.
This is particularly interesting for calibration above or below a flow threshold, for certain months of the year only, etc.
We should put this example in a help page or a vignette.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/43Use of SeriesAggreg on missing values2020-04-09T20:41:18+02:00Delaigue OlivierUse of SeriesAggreg on missing valuesThe function does not work when the time series contain some columns filled entirely with missing data, because a column of NAs is not recognized as numeric.The function does not work when the time series contain some columns filled entirely with missing data, because a column of NAs is not recognized as numeric.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/42Add new function to manage time steps2020-04-05T09:52:46+02:00Delaigue OlivierAdd new function to manage time stepsIt is maybe a good idea to create an internal function to manage the time steps in order to avoid command lines in many functions:
```
if ("daily" %in% class(XXXXXXX)) {
TimeStep <- 60 * 60 * 24
}
if ("hourly" %in% class(XXXXXXX)) {
TimeStep <- 60 * 60
}
```It is maybe a good idea to create an internal function to manage the time steps in order to avoid command lines in many functions:
```
if ("daily" %in% class(XXXXXXX)) {
TimeStep <- 60 * 60 * 24
}
if ("hourly" %in% class(XXXXXXX)) {
TimeStep <- 60 * 60
}
```https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/41Use of SeriesAggreg when TimeLag >= 36002020-07-03T10:23:28+02:00Delaigue OlivierUse of SeriesAggreg when TimeLag >= 3600The `SeriesAggreg()` function does not work when `TimeLag >= 3600`.
```
Error in `*tmp*`[1:(length(Ind2) - 1)] :
only 0's may be mixed with negative subscripts
```The `SeriesAggreg()` function does not work when `TimeLag >= 3600`.
```
Error in `*tmp*`[1:(length(Ind2) - 1)] :
only 0's may be mixed with negative subscripts
```https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/40Set time series of altitudes in CemaNeige2020-03-29T04:59:35+02:00Delaigue OlivierSet time series of altitudes in CemaNeigeIn the `DataAltiExtrapolation_Valery()` function, it would be desirable to be able to provide a time series of altitudes because :
- the geographical position of the meteorological station may change over time
- time series of meteorological data can be computed from the interpolation of several stations, the number of which varies over timeIn the `DataAltiExtrapolation_Valery()` function, it would be desirable to be able to provide a time series of altitudes because :
- the geographical position of the meteorological station may change over time
- time series of meteorological data can be computed from the interpolation of several stations, the number of which varies over timehttps://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/39Different altitudes for meteorological variables in CemaNeige2020-04-30T15:18:37+02:00Delaigue OlivierDifferent altitudes for meteorological variables in CemaNeigeIn the `DataAltiExtrapolation_Valery` function, it would be preferable to be able to set the average elevation of the precipitation and temperature series in two separate variables. Measurements are not always taken at the same geographical position, and therefore at different altitudes.In the `DataAltiExtrapolation_Valery` function, it would be preferable to be able to set the average elevation of the precipitation and temperature series in two separate variables. Measurements are not always taken at the same geographical position, and therefore at different altitudes.https://gitlab.irstea.fr/HYCAR-Hydro/airgr/-/issues/36Avoid checks to increase the speed of use of a function2020-03-19T09:15:04+01:00Thirel GuillaumeAvoid checks to increase the speed of use of a functionSince the many checks in the different functions are quite often the code elements that require a major part of the CPU time, what about proposing a mode that skips all checks?
This would be somehow (but not exactly, as we are not talking about compilation) like the Release and Debug modes of (gfortran) CodeBlocks.
I would see it as a check.mode argument in the functions, if `check.mode == true` we do as usual, and otherwise `check.mode == false` then we skip all of them. The recommandation would then be to do some tests with `check.mode == true` and then run the actual applications with `check.mode == false`. A vos risques et périls. ;)
Just an idea...Since the many checks in the different functions are quite often the code elements that require a major part of the CPU time, what about proposing a mode that skips all checks?
This would be somehow (but not exactly, as we are not talking about compilation) like the Release and Debug modes of (gfortran) CodeBlocks.
I would see it as a check.mode argument in the functions, if `check.mode == true` we do as usual, and otherwise `check.mode == false` then we skip all of them. The recommandation would then be to do some tests with `check.mode == true` and then run the actual applications with `check.mode == false`. A vos risques et périls. ;)
Just an idea...