agroclimatico: Índices y Estadísticos Climáticos e Hidrológicos #599
Comments
|
Thanks for submitting to rOpenSci, our editors and @ropensci-review-bot will reply soon. Type |
|
🚀 Editor check started 👋 |
Checks for agromet (v1.0.0)git hash: d537dd20
(Checks marked with 👀 may be optionally addressed.) Package License: GPL-3 + file LICENSE 1. Package DependenciesDetails of Package Dependency Usage (click to open)
The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate.
Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats(<path/to/repo>)', and examining the 'external_calls' table. baselist (38), c (23), data.frame (18), q (14), file (9), paste0 (8), args (7), dir (6), if (6), length (6), format (5), return (5), suppressWarnings (5), file.path (4), is.finite (4), names (4), seq_along (4), kappa (3), lapply (3), max (3), mean (3), normalizePath (3), source (3), sum (3), url (3), abs (2), as.data.frame (2), call (2), cumsum (2), diff (2), for (2), gamma (2), ifelse (2), is.na (2), log (2), match.call (2), mean.Date (2), min (2), missing (2), rep (2), system.file (2), unique (2), as.character (1), deparse (1), do.call (1), drop (1), exp (1), gsub (1), match.fun (1), mode (1), pmatch (1), range (1), readLines (1), rle (1), seq (1), signif (1), sink (1), strsplit (1), substitute (1), switch (1), tempdir (1) agrometcompletar_serie (5), glo.loglik (4), spi (4), dir_mapas (3), kringe (3), pdsi_coeficientes (3), C_pdsi (2), computar_olas (2), get_mapa (2), mapa_provincias (2), resolve_resolucion (2), agromet_informe (1), anomalia_porcentual (1), borrar_cache (1), compute_breaks (1), ConvertLongitude (1), coord_argentina (1), decil (1), descargar_mapa (1), dias_promedio (1), ith (1), kable_inta (1), lat_label (1), leer_nh (1), leer_surfer (1), lon_label (1), mapa_argentina (1), mapa_argentina_limitrofes (1), mapa_departamentos (1), mapear (1), metadatos_nh (1), olas (1), parglo.maxlik (1), pdsi (1), pdsi_ac (1), pdsi_internal (1), plot.metadatos_nh (1), replace_if_null (1), scale_color_inta (1), spi_params (1), surfer_to_hex (1) utilsdata (23), read.delim (4), capture.output (2), download.file (1), unzip (1) statsstart (10), ts (9), optim (3), end (2), ecdf (1), fitted (1) data.tableas.data.table (4), melt (4), month (4), year (2), rbindlist (1) grDevicescolours (5), palette (3), colorRampPalette (1), rgb (1) magrittr%>% (7) graphicspar (3), points (3) ggplot2geom_sf (2), aes (1), geom_point (1), scale_color_brewer (1) SPEIspi (5) clicli_abort (4) lmomcoare.parglo.valid (4) readrfwf_widths (2), read_fwf (2) gridrasterGrob (2), unit (1) automapautoKrige (2) sfread_sf (1), st_as_sf (1) pngreadPNG (1) rappdirsuser_data_dir (1) tidyrcomplete (1) NOTE: Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately. 2. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
2a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 3.
|
| id | name | conclusion | sha | run_number | date |
|---|---|---|---|---|---|
| 5708281034 | pkgcheck | success | d537dd | 13 | 2023-07-30 |
| 5708281033 | R-CMD-check | success | d537dd | 117 | 2023-07-30 |
| 5708281035 | test-coverage | success | d537dd | 54 | 2023-07-30 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following check_fail:
- no_import_package_as_a_whole
Test coverage with covr
Package coverage: 96.73
Cyclocomplexity with cyclocomp
No functions have cyclocomplexity >= 15
Static code analyses with lintr
lintr found the following 183 potential issues:
| message | number of times |
|---|---|
| Avoid library() and require() calls in packages | 9 |
| Avoid trailing semicolons, they are not needed. | 1 |
| Lines should not be more than 80 characters. | 172 |
| Use <-, not =, for assignment. | 1 |
4. Other Checks
Details of other checks (click to open)
✖️ The following 3 function names are duplicated in other packages:
-
ithfrom bioclim
-
speifrom SPEI
-
spifrom bayestestR, precintcon, SPEI
Package Versions
| package | version |
|---|---|
| pkgstats | 0.1.3.4 |
| pkgcheck | 0.1.2.1 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
|
@ropensci-review-bot assign @Pakillo as editor |
|
Assigned! @Pakillo is now the editor |
|
Gracias @paocorrales y colaboradoras por contribuir su paquete al proceso de revisión de rOpenSci, para el cual actuaré de editor invitado. Cualquier comentario o sugerencia que tengan durante el proceso, por favor no duden en comunicármelo. Algunos puntos que me gustaría tratar antes de comenzar el proceso de revisión: Alcance y solapamiento con otros paquetesEstoy de acuerdo en la amplia utilidad del paquete. Sólo un comentario: cuando se discute el posible solapamiento con otros paquetes se menciona
Igualmente, como sugerencia más que requisito, pienso que tal vez podría ser útil estudiar la compatibilidad con paquetes como ClimInd, que calcula más de 130 índices climáticos diferentes. Por ejemplo, mostrando algunos ejemplos de cómo integrar ambos paquetes para calcular distintos índices. Por último, veo que existe otro paquete llamado agrometR para descargar información de la Red Agroclimática Nacional de Chile (llamada agromet, https://www.agromet.cl/). ¿Piensan que la similitud en el nombre (agromet / agrometR) y tema (clima) de ambos paquetes podría ser problemático o confuso para futuros usuarios? En tal caso, ¿se plantearían un cambio de nombre del paquete? (ver guía rOpenSci). DependenciasEl chequeo automático sugiere que "Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately." Una búsqueda rápida en el código parecen indicar que los paquetes señalados (scales, ggnewscale, kableExtra, Rcpp) sí que se emplean, por lo que creo que podemos ignorar este comentario. Nombres de funcionesHay algunas funciones con nombre duplicado respecto a otros paquetes:
Me preocupa particularmente la duplicidad de nombres con el paquete SPEI (funciones DocumentaciónCreo que sería muy útil generar ya una web del paquete para facilitar el proceso de revisión. Esta web puede generarse automáticamente usando En el README, el tutorial (vignette) y algunas funciones se habla de datos en formato NH y estaciones NH. Estaría bien clarificar a qué se refiere NH. Por favor añadan unas directrices sobre cómo contribuir (ver https://devguide.ropensci.org/collaboration.html?q=contributi#contributing-guide). Chequeo del paqueteTras ejecutar Warning: [escalas.R:16] @Format requires a value Integración ContinuaPor favor modifiquen su GitHub Action para chequear el paquete en la versión actual de R, la anterior, y la que viene (ver guía). E idealmente tanto en Windows como Mac como Ubuntu. Para ello podría usarse esta plantilla generada por RevisoresPor favor sugieran el nombre de varios revisores potenciales del paquete. Muchas gracias |
|
Buenos días @paocorrales Sólo quería comprobar que recibieron mi mensaje anterior y saber si está todo claro. Si no es así, necesitan ayuda con algo, o simplemente necesitan más tiempo, por favor avísenme. En cuanto resolvamos estas cuestiones preliminares podremos empezar la revisión con revisores externos Gracias y saludos |
|
Muchas gracias por los comentarios y recomendaciones @Pakillo! Me disculpo por la demora, estaba de viaje pero esta semana voy a revisar todo en detalle y hacer los cambios necesarios para comenzar con la revisión. Saludos! |
|
¡Estupendo, gracias! |
|
Muchas gracias por los comentarios @Pakillo y perdón por la demora, respondo a cada punto entre lineas.
Muchas gracias por mencionar esto, si bien aún no está en CRAN voy a editar mi primer mensaje incluyendo el link al repositorio para que no genere confunción.
Muchas gracias por la sugerencia! Estuvimos explorando este paquete y si bien incluye muchas funciones en varios casos vimos que las funciones son algo repetitivas (por ejemplo muchas funciones para calcular la temperatura mayor a distintos umbrales, en vez de 1 función que haga eso para cualquier umbral arbitrario). Por otro lado, la salida de estas funciones no es siempre compatible con la filosofía tidy que usamos en nuestras funciones. De todas maneras lo vamos a seguir explorando!
Luego de consultar con el equipo decidimos cambiar el nombre del paquete a agroclimatico. No encontramos otros paquetes de R o software con este nombre y esperamos que genere menos confución. Quedo atenta por si hay que hacer cambios en este issue.
Entendemos la sugerencia, spi es una función presente en varios paquetes y además es el nombre que se usa para hacer referencia al índice aún en la literatura en español. Cambiamos el nombre de la función a spi_indice que si bien es algo redundante puee ayudar a evitar la confución.
Gracias por la sugerencia, el paquete ahora tiene una web creada con pkgdown: https://agrometeorologiainta.github.io/agroclimatico/
Agregamos algunas aclaraciones.
Las directrices sobre como contribuir están disponibles en https://github.com/AgRoMeteorologiaINTA/agromet/blob/master/.github/CONTRIBUTING.md y linkeadas al README
Warnings corregidos, gracias!
Actualizamos la GitHub Action para chequear el paquete con distintas versiones de R y sistemas operativos. En el Caso de Mac tuvimos que skipear los test visuales porque fallan debido a una incompatibilidad entre sistemas operativos que no debería afectar a le usuarie.
Sugerimos a Ana Laura Dietrich, ella tiene experiencia en el desarrollo de paquetes y en el área agrocliomática. |
|
@ropensci-review-bot check package |
|
Thanks, about to send the query. |
|
🚀 Editor check started 👋 |
|
@ropensci-review-bot help |
|
Hello @Pakillo, here are the things you can ask me to do: |
Checks for agroclimatico (v1.0.0)git hash: 2d86dbe8
(Checks marked with 👀 may be optionally addressed.) Package License: GPL-3 + file LICENSE 1. Package DependenciesDetails of Package Dependency Usage (click to open)
The table below tallies all function calls to all packages ('ncalls'), both internal (r-base + recommended, along with the package itself), and external (imported and suggested packages). 'NA' values indicate packages to which no identified calls to R functions could be found. Note that these results are generated by an automated code-tagging system which may not be entirely accurate.
Click below for tallies of functions used in each package. Locations of each call within this package may be generated locally by running 's <- pkgstats::pkgstats(<path/to/repo>)', and examining the 'external_calls' table. baselist (38), c (23), data.frame (18), q (14), file (9), paste0 (8), args (7), dir (6), if (6), length (6), format (5), return (5), suppressWarnings (5), file.path (4), is.finite (4), names (4), seq_along (4), kappa (3), lapply (3), max (3), mean (3), normalizePath (3), source (3), sum (3), url (3), abs (2), as.data.frame (2), call (2), cumsum (2), diff (2), for (2), gamma (2), ifelse (2), is.na (2), log (2), match.call (2), mean.Date (2), min (2), missing (2), rep (2), system.file (2), unique (2), as.character (1), deparse (1), do.call (1), drop (1), exp (1), gsub (1), match.fun (1), mode (1), pmatch (1), range (1), readLines (1), rle (1), seq (1), signif (1), sink (1), strsplit (1), substitute (1), switch (1), tempdir (1) agroclimaticocompletar_serie (5), glo.loglik (4), dir_mapas (3), kringe (3), pdsi_coeficientes (3), C_pdsi (2), computar_olas (2), get_mapa (2), mapa_provincias (2), resolve_resolucion (2), agromet_informe (1), anomalia_porcentual (1), borrar_cache (1), compute_breaks (1), ConvertLongitude (1), coord_argentina (1), decil (1), descargar_mapa (1), dias_promedio (1), ith (1), kable_inta (1), lat_label (1), leer_nh (1), leer_surfer (1), lon_label (1), mapa_argentina (1), mapa_argentina_limitrofes (1), mapa_departamentos (1), mapear (1), metadatos_nh (1), olas (1), parglo.maxlik (1), pdsi (1), pdsi_ac (1), pdsi_internal (1), plot.metadatos_nh (1), replace_if_null (1), scale_color_inta (1), spi_indice (1), spi_params (1), surfer_to_hex (1) utilsdata (23), read.delim (4), capture.output (2), download.file (1), unzip (1) statsstart (10), ts (9), optim (3), end (2), ecdf (1), fitted (1) data.tableas.data.table (4), melt (4), month (4), year (2), rbindlist (1) grDevicescolours (5), palette (3), colorRampPalette (1), rgb (1) magrittr%>% (7) graphicspar (3), points (3) ggplot2geom_sf (2), aes (1), geom_point (1), scale_color_brewer (1) SPEIspi (5) clicli_abort (4) lmomcoare.parglo.valid (4) readrfwf_widths (2), read_fwf (2) gridrasterGrob (2), unit (1) automapautoKrige (2) sfread_sf (1), st_as_sf (1) pngreadPNG (1) rappdirsuser_data_dir (1) tidyrcomplete (1) NOTE: Some imported packages appear to have no associated function calls; please ensure with author that these 'Imports' are listed appropriately. 2. Statistical PropertiesThis package features some noteworthy statistical properties which may need to be clarified by a handling editor prior to progressing. Details of statistical properties (click to open)
The package has:
Statistical properties of package structure as distributional percentiles in relation to all current CRAN packages
All parameters are explained as tooltips in the locally-rendered HTML version of this report generated by the The final measure (
2a. Network visualisationClick to see the interactive network visualisation of calls between objects in package 3.
|
| id | name | conclusion | sha | run_number | date |
|---|---|---|---|---|---|
| 7095108109 | pages build and deployment | success | f43114 | 10 | 2023-12-05 |
| 7095086203 | pkgcheck | success | 2d86db | 21 | 2023-12-05 |
| 7095086206 | pkgdown | success | 2d86db | 8 | 2023-12-05 |
| 7095086213 | R-CMD-check | success | 2d86db | 125 | 2023-12-05 |
| 7095086208 | test-coverage | success | 2d86db | 62 | 2023-12-05 |
3b. goodpractice results
R CMD check with rcmdcheck
R CMD check generated the following check_fail:
- no_import_package_as_a_whole
Test coverage with covr
Package coverage: 96.73
Cyclocomplexity with cyclocomp
No functions have cyclocomplexity >= 15
Static code analyses with lintr
lintr found the following 190 potential issues:
| message | number of times |
|---|---|
| Avoid library() and require() calls in packages | 9 |
| Avoid trailing semicolons, they are not needed. | 1 |
| Lines should not be more than 80 characters. | 179 |
| Use <-, not =, for assignment. | 1 |
4. Other Checks
Details of other checks (click to open)
✖️ The following function name is duplicated in other packages:
-
ithfrom bioclim
Package Versions
| package | version |
|---|---|
| pkgstats | 0.1.3.9 |
| pkgcheck | 0.1.2.11 |
Editor-in-Chief Instructions:
This package is in top shape and may be passed on to a handling editor
|
Muchas gracias por la respuesta y todas las mejoras al paquete @paocorrales et al. Yo creo que está casi listo para pasar a revisión. Solo tres pequeños detalles por ahora:
Procedo a continuación a buscar revisores. Si pueden arreglar estos pequeños detalles mientras tanto, estupendo. |
Editor checks:
Editor comments¿Algunos issues parecen obsoletos y se podrían cerrar? Sería bueno cerrar lo que ya esté resuelto para indicar claramente a los revisores y usuarios/as del paquete el estado actual del paquete y cuáles son los temas abiertos. ¡Gracias! |
|
@ropensci-review-bot seeking reviewers |
|
Please add this badge to the README of your package repository: [](https://github.com/ropensci/software-review/issues/599)Furthermore, if your package does not have a NEWS.md file yet, please create one to capture the changes made during the review process. See https://devguide.ropensci.org/releasing.html#news |
Pakillo
changed the title
|
Muchas gracias @Pakillo, resolví todos los detalles mencionados 😃 |
|
Estupendo, muchas gracias @paocorrales |
|
@ropensci-review-bot check package |
|
Thanks, about to send the query. |
|
🚀 Editor check started 👋 |
|
📆 @pmnatural you have 2 days left before the due date for your review (2024-02-06). |
|
Con algunos problemas al tener que cambiar de equipo. Estoy rehaciendo la revision en una maquina distinta y con mala internet. Espero subir el reporte mas tarde. |
Revisión del paquete AgroclimaticoPor favor describe cualquier relación de trabajo que tengas/hayas tenido con los autores del paquete) No he tenido ninguna relacion de trabajo con los autores del paquete.
DocumentaciónEl paquete incluye todos los siguiente tipos de documentación:
Funcionalidad
Estimación de horas dedicadas a la revisión:
Comentarios de la revisiónLos items sin tildar no han sido objeto de esta revision Seria util que despues de la descripcion inicial del paquete haya una lista de todas las funciones disponibles a la fecha de la version. Segun la viñeta estas serian: En la viñeta estan: En los man pages se suman estas ; Segun la descripcion del paquete, se asume que es solo para personal del INTA aunque otros usuarios externos podrian tambien beneficiarse. Para ello seria importante, si lo consideran conveniente los autores, incluir un breve comentarios sobre el tipo de archivo, estructura de datos y nombres de columnas que deberian tener estos datos. |
|
¡ Muchas gracias por su revisión @pmnatural ! Antes de continuar, ¿podría indicar una estimación del tiempo (horas) dedicado a la revisión? Para efectos puramente estadísticos del programa de revisión de rOpenSci Gracias |
|
Gracias Priscilla por la revisión! Respecto a tu comentario te consulto si la propuesta sería incorporar toda la lista de funciones en el README. Ahora no son muchas funciones y se podría hacer pero con suerte en el futuro espero sumar más y creo que quedaría muy largo. Una alternativa que se me ocurre es linkear desde el README al índice de la web del paquete donde está la lista de funciones: https://agrometeorologiainta.github.io/agroclimatico/reference/index.html Te parece una alternativa posible? |
Revisión de un paqueteNo tengo ninguna relación de trabajo con los autores
DocumentaciónEl paquete incluye todos los siguiente tipos de documentación:
Funcionalidad
Estimación de horas dedicadas a la revisión: 8
Comentarios de la revisiónEn primer lugar, quiero agradecer la oportunidad de revisar este paquete y espero que los comentarios sirvan para mejorar en los puntos que los autores consideren oportunos. Creo que los autores han hecho un gran trabajo compilando las funciones relacionadas con la gestión de datos climáticos que son útiles para el INTA. He revisado el paquete desde el punto de vista de un usuario y tengo algunos comentarios generales y también específicos de cada función. Incluyo en la revisión código que he ido ejecutando y con comentarios en partes que no han funcionado correctamente y reflexiones. Creo que algunos aspectos formales de la documentación pueden ser mejorados. Para empezar, la funcionalidad del paquete no está completamente definida en la documentación (Readme). El paquete calcula índices y estadísticos climáticos e hidrológicos a partir de datos tidy, pero también permite representar esta información y otra en mapas en Argentina, y permite leer datos climáticos en el formato del INTA. Creo que toda esta información debería de estar incluida en el Readme. En varios puntos de la documentación de las funciones (ver debajo) he detectado que no se especifica el formato concreto en el que los argumentos deben declararse y también argumentos cuya descripción es confusa. Mejorar estas descripciones e incluir programación defensiva relativa a los argumentos (en general faltante) ayudaría mucho a los usuarios. Creo que la funcionalidad de spi_indice y spei_indice está ya contenida en el paquete SPEI. A menos que se aclare cual es la novedad de las funciones escritas para agroclimático, sugiero eliminar estas funciones. Además, no he conseguido correr las funciones scale_color/fill_inta y pdsi/pdsi_ac (ver debajo los comentarios). Finalmente, en el logotipo del paquete pone "AgroMET" lo que es un poco raro si el paquete se llama "agroclimatico". dias_promedioNo calcula ningun estadístico meteorológico como dice la descripción si no que extrae dia, mes y dia juliano de un set de datos. Previamente el usuario puede haber extraido los días que representan un evento meteorológico de interés, pero no tiene por qué. La descripción de la funcionalidad no es clara en la ayuda. Sugiero poner "Calcula el valor promedio anual del primer y ultimo dia para una serie de fechas dada". En los ejemplos habría que cambiar "summarise" por "reframe" como aparece en el Readme. En "Value" se dice que el output tiene "variables extras en el caso de hacer el cálculo para distintos grupos", pero ¿cómo se puede especificar para qué grupos quieres el cálculo? Erratas en la documentación: "seria de fechas" completar_serieNo se especifica el formato del vector fecha ni de rango en la documentación. decil y anomalia_porcentualEn la descripción se necesita incluir una definición más técnica de qué valores se devuelven, es decir, que hace la función decil y la anomalia_porcentual y referencias. En estadística, los deciles de una serie de datos son estadísticos descriptivos que representan los valores que limitan la serie de datos en 10 partes. Al aplicar la función sin tener mucho detalle en la descripción yo hubiera esperado obtener eso. Sin embargo, en el ejemplo ejecutado debajo obtengo un vector de igual longitud de la serie de datos. Sugiero mejorar la descripción en este sentido. La anomalía porcentual ¿respecto a qué valor de la serie de referencia se calcula? Debería especificarse. ¿Por qué para calcular la anomalia es necesario especificar que no se consideren los NA, pero para decil no? Parece una cuestión de inconsistencia en cómo están escritas las funciones. scala_temp/pp_XXXLa documentación está incompleta. No hay ejemplos de uso ni se entiende bien que hacen estas funciones. Tampoco se especifica qué valores retorna. ithFunciona correctamente y está bien documentada. metadatos_nhPara facilitar la comprensión a los usuarios convendría especificar que son "estaciones NH". La descripción es incompleta en el sentido de que sólo se especifica una parte de la información que devuelve la función. Erratas en la documentación: "provincia o provincia", "especificar" debería ser "específicas". kable_intaSustituir "Setear" por "Formatear" o similar. Funciona correctamente. leer_surferEn el ejemplo se sobreescribe un objeto con el nombre de otra función implementada en el paquete (scala_temp/pp_XXX). Debería estar explicado en algún sitio cual es el link entre ambas. Erratas en la documentación: "laeer". mapa_XXXAñadiría en la descripción los posibles valores para el argumento "provincias". mapearErratas en la documentación: "veriable", "a el", "masyores", "1500m". El argumento "breaks" no está bien definido. Habría que especificar que se refiere a que hay que introducir valores (¿uno o varios?) en el rango del argumento "valor". No se explican las funciones coord_argentina ni theme_inta_mapa en ningún sitio. ¿Se utilizan sólo internamente? En ese caso no deberían estar exportadas. olas y umbralesNo se explica en la documentación como hay que especificar los umbrales. Es poco habitual que se pueda poner cualquier palabra como nombre de un argumento y una mala praxis porque se puede sobreescribir algo importante en la función. Sería más adecuado que hubiera un argumento para los umbrales y que éstos se pudieran introducir en forma de lista con nombres (ej. umbral = list(calor = "t_max > 20")) Corregir algunas erratas en la documentación de olas: "mbral", "drtn" Corregir algunas erratas en la documentación de umbrales: " pdsi/pdsi_acNo funciona indicandole los argumentos no definidos por defecto. Habría que incluir en la documentación las referencias que se citan e incluir las unidades de la precipitacion y la etp. Es redundante incluir un argumento para los coeficientes en forma de lista y luego argumentos para cada coeficiente por separado. Sugiero eliminar una de las dos formas. Corregir algunas erratas de la ayuda: "list de coeficientes", "El el cálculo". scale_color/fill_inta pdsi/pdsi_acEl ejemplo no funciona. Tampoco funciona otro ejemplo que he creado siguiendo las instrucciones de la documentación. Corregir erratas en la documentación: "escala.)", "tiene prioridad por sobre", "deinidos" spei_indice/spi_indiceLa funcionalidad está repetida respecto a las funciones spi y spei del paquete SPEI. A menos que se aclare qué aporta esta función sugiero eliminarla y crear una viñeta que incluya cómo se usa el paquete SPEI con datos en el formato del INTA. Si he entendido bien, las fechas deben introducirse como meses ¿no? Por favor, especificar esto si se mantiene la función. En la documentación no se entiende bien que es el argumento "escalas" y no se cita el paquete SPEI. |
|
¡Muchas gracias @VeruGHub por su detallada revisión! Seguro que será muy útil. |
|
@ropensci-review-bot submit review #599 (comment) time 8 |
|
Logged review for VeruGHub (hours: 8) |
|
@ropensci-review-bot submit review #599 (comment) |
|
I'm sorry human, I don't understand that. You can see what commands I support by typing:
|
|
@ropensci-review-bot submit review #599 (comment) time NA |
|
Logged review for pmnatural (hours: NA) |
ropensci-review-bot
added
4/review(s)-in-awaiting-changes
and removed
3/reviewer(s)-assigned
labels
|
Estimada @paocorrales y coautores: Ya tenemos las revisiones de @pmnatural y @VeruGHub, a quienes agradezco su esfuerzo. Espero que sus comentarios sean útiles. Añado algunos comentarios al respecto: READMEAmbas revisoras comentan que les gustaría ver más información sobre el paquete ahí. Yo la veo correcta (sobre todo al contar con un magnífico tutorial complementario (vignette)), pero tal vez se pueda añadir algo más o re-estructurar la información del README para que quede más claro, al menos en cuanto a los grandes bloques de funcionalidad del paquete (lectura de datos INTA, indicadores, mapeo, etc). Una sugerencia para que quede más clara la funcionalidad global del paquete es agrupar las funciones en el índice por temáticas (ver https://r-pkgs.org/website.html#index-organization). En el primer párrafo del Readme hay una errata ( En el segundo párrafo del Readme, la función En el tercer párrafo se menciona por primera vez el INTA. Es posible que muchos no conozcan a qué se refiere el acrónimo, por lo que añadiría el nombre completo del instituto y un enlace a la web oficial. Es cierto que el logo refleja el antiguo nombre del paquete (agromet), ¿tal vez sea buena idea modificarlo? La 'vignette' a mí si me funciona, siempre que se especifique dias_promedioEn esta y otras funciones, como indica @VeruGHub, sería conveniente reemplazar decilEn la función decil, pienso que sería útil dar la opción de devolver tanto el número del decil (1, 2, 3, ...) como el valor numérico (ej. 7.7 correspondente al decil 1, etc). Por ejemplo viendo el ejemplo en https://agrometeorologiainta.github.io/agroclimatico/articles/estadisticas-e-indices-climaticos.html#deciles no me queda claro cómo interpretar en qué decil nos encontramos simplemente viendo el valor de la columna 'decil'. mapearEsta función me parece muy útil pero me pregunto si podría rediseñarse para que el primer argumento sea un data.frame o tibble, siguiendo la costumbre en tidyverse. Esto permitiría llamarla directamente dentro de un pipe, p. ej.: datos_aleatorios |>
mapear(pp, lon, lat,
cordillera = TRUE,
escala = escala_pp_diaria,
variable = "mm",
titulo = "Precipitación aleatoria",
fuente = "Fuente: datos de ejemplo")olas y umbralesEstas funciones también me gustan mucho y me parecen muy útiles, pero igualmente me pregunto (a modo de reflexión) si el primer argumento podría ser un data.frame o tibble, dado que se usan varias variables del mismo para calcular las olas. Esto podría permitir además que no fuese necesario usar summarise/reframe, p. ej: olas(NH0358, calor = t_max > 20, frio = t_min <= 0)SPI, SPEITambién aquí me planteo si no sería conveniente (siguiendo la filosofía tidyverse) que el primer argumento fuera siempre un data.frame o tibble. De ese modo no habría que usar Estoy de acuerdo con @VeruGHub en que sería bueno explicar la utilidad de estas funciones y en qué difiere esta implementación de la del paquete SPEI (utilizado internamente). GeneralEn los ejemplos sería conveniente simplificar los datasets al máximo, dejando solamente lo necesario para mostrar lo que hace la función. Por ejemplo, en la función ErratasHay algunas erratas en la documentación. Podría usarse el paquete spelling o herramientas similares para facilitar su detección y corrección. Nada más por ahora. Espero que mis comentarios, unidos a los de las revisoras, sean útiles para revisar el paquete. Por favor no duden en comentar cualquier duda o discrepancia que puedan tener. Muchos de los cambios propuestos son sugerencias o invitaciones a la reflexión. Quedo pues a la espera de su revisión. Saludos cordiales Paco |
|
@paocorrales, @eliocamp, @yabellini, @NatiGattinoni: please post your response with Here's the author guide for response. https://devguide.ropensci.org/authors-guide.html |
|
Estimadas @paocorrales, @eliocamp, @yabellini, @NatiGattinoni: Espero que vaya todo bien. Me preguntaba cómo iba avanzando la revisión del paquete. Si puedo ayudar en algún aspecto o tienen cualquier duda o comentario no duden en decirlo. Gracias |
|
Hola! En primer lugar, muchas gracias @pmnatural y @VeruGHub por la revisión y los comentarios, y a @Pakillo por ordenarlos y organizar todo. Incorporé los comentarios y sugerencias al paquete. El detalle fino de los cambios se puede seguir en los issues del repositorio del paquete (1 issue por función o familia de funciones). Respondiendo a los comentarios específicos:
Mejoré la documentación para explicar mejor el uso de la función junto con los verbos de dplyr. Corregí errores en los ejemplos (ahora usa reframe()). Incorporé un nuevo ejemplo que muestra como usar la función agrupando datos. Para esto el paquete tiene un nuevo set de datos que corresponde a una serie temporal de una estación meteorológica similar al que ya se estaba usando.
Había un error en el código. Ahora funciona tanto si la resolución es "dia" o "1 dia". Mejoré la documentación agregando los detalles a los argumentos mencionados.
Mejore la documentación para explicar mejor la salida de la función. Respecto a la diferencia entre las funciones, efectivamente ambas son internamente distintas y deciles() no requiere el argumento na.rm. Las documentamos en conjunto ya que se suelen usar en conjunto y encontramos muchos ejemplos donde funciones documentadas como "familia" comparten algunos pero no todos los argumentos documentados.
Agregue ejemplos en la documentación y lo que devuelve. Los ejemplos usan los nuevos datos del paquete y son mas simples.
Para esta y otras funciones revisé el output de los ejemplos y cuando eran muy largos ahora solo muestra las primeras 10 lineas.
Arreglé los errores de tipeo y extendí la documentación para incluir información sobre metadatos.
Documetación arreglada.
Documentación arreglada y mejores ejemplos (asociadas a las funciones scale_*())
Gracias por la sugerencia, ahora incluye la lista de provincias.
Arreglé los errores de tipeo e incluí las aclaraciones pedidas. Además ahora la función
Corregí los errores de tipeo (drtn es un tipo de dato que representa diferencia entre tiempos). No cambié el comportamiento de las funciones
Extendí la documentación con todo lo mencionado. Además incluí la necesidad de tener una serie larga de datos para calcular el indice (no es suficiente con 2 datos). La función pdsi_coeficientes() ahora se documenta aparte para no generar confución.
Mejoré la documentación de esta familia de funciones, en primer lugar indicando que son escalas de colores para variables discretas (no estaba explicito) y mejoré los ejemplos usando un nuevo set de datos con valores de precipitación mensual. Esto simplifica los ejemplos y son más realistas.
Modifiqué la documentaición incluyendo lo solicitado. Una de las ventajas principales de estas funciones y que justifican su existencia es que devuelven el resultado como un data.frame que se puede usar de manera directa para el análisis de datos con dplyr. Además de lo mencionado anteriormente:
Espero no haberme olvidado de nada, aguardo sus comentarios! |
|
Muchas gracias @paocorrales ! Estoy revisando todos los cambios. Me parece que la página web del paquete no está actualizada a la última versión, ¿cierto? Veo que el GitHub Actions de pkgdown está desactivado. Por favor actívelo o regenere la web con pkgdown manualmente. Nos ayudaría ver la web actualizada para la revisión ¡Gracias! |
|
La web ahora está actualizada y la GitHub action activada. Gracias! |
|
¡Muchas gracias @paocorrales por la revisión! Creo que el paquete y la documentación han mejorado muchísimo. Algunos comentarios a la última versión: decilEs posible que no esté entendiendo bien esta función, pero encuentro su valor de salida un poco confuso. Por ejemplo, en este caso, x = 1:10
decil(x)Pero en este caso devuelve valores con decimales desde 0 hasta 10: x <- seq(0, 19.5, 0.5)
decil(x)Pienso que estos valores con decimales podrían generar confusión. Por ejemplo, alguien que quiera saber en qué decil cae cada dato podría intentar hacer un round(decil(x))cuando probablemente necesite ejecutar ceiling(decil(x))¿Cuál es la ventaja de devolver números decimales como salida de @VeruGHub también planteaba esta cuestión: ¿Por qué para calcular la anomalia es necesario especificar que no se consideren los NA, pero para decil no? Entiendo que la función Por ejemplo, la función x <- 1:10
x[1] <- NA
quantile(x, probs = seq(0, 1, 0.1))Si especificamos quantile(x, probs = seq(0, 1, 0.1), na.rm = TRUE)Al igual que @VeruGHub, pienso que la función dias_promedioEsta función ha quedado muy bien, y la documentación y los ejemplos son muy claros. ¡Gracias! umbralesEl último ejemplo requiere cambiar de olasCreo que sería bueno incluir un argumento Por ejemplo, esta ola de calor de 22 días de duración: data(NH0358)
NH0358 %>%
reframe(olas(fecha, calor = t_max > 20)) %>%
slice_head(n = 1)se transforma en otra de 19 días de duración si falta el dato de temperatura del 20 de enero: NH0358 %>%
mutate(t_max = if_else(fecha == "1951-01-20", NA, t_max)) |>
reframe(olas(fecha, calor = t_max > 20)) %>%
slice_head(n = 1)pero vuelve a tener 22 días de duración si falta la fila completa del 20 de enero (cuando en realidad ese día la temperatura pudo ser <20ºC): NH0358 %>%
filter(fecha != "1951-01-20") |>
reframe(olas(fecha, calor = t_max > 20)) %>%
slice_head(n = 1)Pienso que para evitar posibles inconsistencias, podría añadirse un nuevo argumento Por otro lado, es una decisión de los autores, pero como sugerencia, ¿tal vez utilizar “duración” en lugar de “longitud” para especificar la duración (en días) de la ola? ithEl ejemplo devuelve todos los valores de ith como NA, dado que NH0358 %>%
filter(!is.na(hr)) |>
mutate(t_media = (t_max + t_min)/2) %>%
mutate(ith = ith(t_media, hr)) %>%
slice_head(n = 10)PDSIEl ejemplo usa datos <- data.frame(fecha = seq(as.Date("1985-01-01"),
as.Date("2015-12-01"),
by = "1 month"))
set.seed(42)
datos |>
mutate(pp = rgamma(nrow(datos), shape = 2, scale = 10),
etp = rgamma(nrow(datos), shape = 1, scale = 3),
pdsi_ac = pdsi(pp, etp)) |>
slice_head(n = 10)(modificar también el ejemplo de SPICreo que sería bueno que los ejemplos de esta función siguieran el estilo utilizado en el tutorial, esto es, usando He corregido algunas erratas en este pull request. Espero que mis comentarios/sugerencias sean claros y fáciles de solucionar. Quedo a la espera de estos cambios, ¡y ya casi estaríamos! Gracias |
|
Hola @Pakillo! Gracias por los comentarios. Respondo a cada uno por separado. En el caso de olas(), la función incluye ahora los cambios pedidos y además se resuelve un error que surgió de los ejemplos. La función decil() no tiene cambios aún pero con @eliocamp damos más detalles abajo y quedo atenta a sus respuestas. decil()La función Este tipo de transformación es útil en ciertas comparaciones entre observaciones y modelos o entre múltiples modelos, ya que muchas veces los modelos tienen algún sesgo en la media o la varianza pero aún así representan bien la variabilidad relativa. Cambiar el valor de retorno a valores redondeados implicaría cambiar la función de una transformación a una discretización, perdiendo información. decil_x <- decil(rnorm(100))
cut(decil_x, seq(0, 10, by = 1)) # discretizar a deciles
cut(decil_x, seq(0, 10, by = 0.1)) # discretizar a percentiles
cut(decil_x, seq(0, 10, by = 2)) # discretizar a quintilesLa función no requiere trato especial de los valores faltantes porque al estimar la curva de densidad de probabilidad acumulado, éstos se descartan automáticamente (ver También se podría devolver todo umbrales()Ejemplo arreglado olas()El ejemplo que presentas es muy interesante porque detecta un problema que no habíamos tenido en cuenta. La inconsistencia que detectas no se debe a la presencia de NAs explicitos, si no que se debe a un dato faltante implicito, la falta de un día dentro de la serie de datos. Agregamos un test dentro de la función que chequea que la serie que recibe está completa. Si no está completa devuelve un error. Respecto al comportamiento para NAs, sumamos un argumento remplaza.na que por defecto sigue devolviendo el mismo resultado. Esto es consistente con la función rle() de base. Sin embargo si es TRUE, la función reemplazara los NA por el valor previo en la serie "sumando" ese valor a la ola o persistencia. ith()Gracias, ejemplo arreglado. PDSIGracias por la recomendación! SPIEstos ejemplos también están actualizados |
|
¡Muchas gracias por la respuesta y los cambios implementados! Me alegro de que todo haya tenido fácil solución. Gracias también por explicar lo que hace la función Tras esta propuesta para su consideración parte de los autores, invito de nuevo a @pmnatural y @VeruGHub a revisar el paquete y los cambios aquí discutidos. Por favor, comuniquen si tienen más observaciones que hacer o si no manifiesten su conformidad usando esta plantilla. ¡Gracias! |
Pakillo
added
5/awaiting-reviewer(s)-response
and removed
4/review(s)-in-awaiting-changes
labels
|
Hola @pmnatural @VeruGHub Cuando puedan por favor revisen si están conformes con los cambios realizados en el paquete y manifiesten su conformidad usando esta plantilla. Gracias |
|
Hola @paocorrales et al: Muchas gracias por considerar los cambios propuestos. El readme y la documentación y ejemplos de las funciones han mejorado mucho y los cambios realizados han aclarado mis dudas. Disculpad @Pakillo y @paocorrales la tardanza de mi respuesta. He detectado algunos puntos más que se pueden mejorar, por si se quieren considerar:
Eso es todo por mi parte, enhorabuena por el paquete. Un saludo |
|
Hola @paocorrales et al. Creo que el paquete está prácticamente listo para ser aceptado. No sé qué opinan de las últimas sugerencias (renombrar decil por cuantil o similar, traducir days al español...). En cuanto me digan su opinión sobre estos últimos comentarios propondré la aceptación del paquete. Quedo a la espera, muchas gracias |
|
Hola y muchas gracias nuevamente por los comentarios. Voy respondiendo a cada uno:
|
Nombre de la Persona Encargada: Paola Corrales
Usuario GitHub de la Persona Encargada: @paocorrales
Usuario GitHub de las Otras Autoras del Paquete: @eliocamp, @yabellini, @NatiGattinoni
Repositorio: https://github.com/AgRoMeteorologiaINTA/agroclimatico
Versión Enviada: 1.0.0
Tipo de Envio: Estándar
Editora: @Pakillo
Revisores: @VeruGHub, @pmnatural
Archivo: TBD
Versión Aceptada: TBD
Idioma: es
Alcance
Por favor, indica qué categoría(s) aplican a este paquete. Las puedes encontrar en nuestras políticas de inclusión de paquetes (Inglés). Por favor, tilda todas las apropiadas. Si no estás seguro, te sugerimos que comiences un pre-envío.
Explica cómo y por qué el paquete encaja dentro de estas categorías (1 a 3 oraciones):
El paquete agromet incluye una serie de funciones para trabajar con datos meteorológicos y calcular distintos índices asociados a aplicaciones agrometeorológicas. Los datos de entrada se organizan usando la filosofía de datos ordenados (o tidy), por lo que las funciones del paquete son genéricas. Pueden aplicarse a cualquier conjunto de datos tabulares independientemente de su origen, orden o nombres de columna.
¿Cuál es la audiencia esperada y las aplicaciones científicas de este paquete?
El paquete es ampliamente utilizado por usuaries en distintas dependencias del Instituto Nacional de Tecnología Agropecuaria (INTA) que brindan servicios e información a los agricultores de Argentina. Sin embargo, les usuaries del paquete se extienden a otras instituciones y países de América Latina ya que casi todas las funciones están desarrolladas para ser de uso general e independiente de los datos recolectados por el INTA y la documentación está escrita integramente en español.
¿Hay otros paquetes de R que logren el mismo objetivo? Si los hay, ¿cómo se diferencian del tuyo, o alcanzan nuestro criterio del mejor de su categoría (documento en Inglés)?
Existen algunos paquetes cómo frost y meteor que incluyen funciones para utilizar datos meteorológicos y calcular indices que en algunos casos están asociadosa la actividad agropecuaria. Otro paquete a mencionar es agroclim que según la documentación también incluye el cálculo de una serie de índices agrometeoroógicos fue archivado por CRAN en abril de este año pero el código esta disponible en un repositorio público.
agromet se diferencia de estos paquetes principalmente por 2 características. Por un lado su documentación está escrita integramente en español lo que facilita su uso en Sudamérica. Por el otro lado, sus funciones estan pensadas para ser compatibles con el ecosistema de paquetes que utilizan la filosofía "tidy", lo que facilita posterior manipulación, visualización y uso de los resultados obtenidos con agromet.
(Si aplica) ¿Tu paquete cumple con nuestras guías de Ética, Privacidad de Datos e Investigación de Sujetos Humanos (documento en Inglés)?
No aplica.
Si ya has hecho una consulta de pre-envío, por favor pega el enlace al issue correspondiente, una publicación del foro, u otra discusión. Alternativamente, etiqueta al editor (con @tag) con el que te contactaste.
No aplica.
(Si aplica) Explique las razones de los elementos
pkgcheckque su paquete no puede pasar.Revisiones Técnicas
Tilda los siguientes items para confirmar que los has completado:
Este paquete:
Opciones de Publicación
¿Tienes intenciones de subir este paquete a CRAN?
¿Tienes intenciones de enviar este paquete a Bioconductor?
¿Deseas enviar un Artículo de Aplicaciones sobre tu paquete a Methods in Ecology and Evolution (documento en Inglés)? Si es así:
Opciones para MEE
Código de Conducta
The text was updated successfully, but these errors were encountered: