Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bring back suggested function #10

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Bring back suggested function #10

wants to merge 15 commits into from

Conversation

crcruzr
Copy link
Collaborator

@crcruzr crcruzr commented Jun 25, 2024

Sorry for the pull request. I merged the files by mistake and brought them back for your revision.

@crcruzr
Copy link
Collaborator Author

crcruzr commented Jun 25, 2024

I have added different comments to guarantee that the function works with the code. I needed to modify some files in the package and add documentation files to ensure the GitHub validations were done well.

Now, the validation process fails because the files "mamm_dep_col_conf.csv" and "mamm_mun_col_conf.csv" generate an error. I suggest that those files be adjusted to guarantee the package's functionality.

Regards,

Cristian

@dlizcano
Copy link
Owner

dlizcano commented Jul 4, 2024

ok

Copy link
Owner

@dlizcano dlizcano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no se si ahora si te salen los comentarios... si no te los envio como foto al email.

@@ -134,7 +133,7 @@ mamm_coords_validator <- function(df, sp_names, taxon = NULL, colmap = NULL, lon
if (nrow(vect.spp.i) > nrow(vect.spp.i.t) ) {

vect.spp.i.novali <- vect.spp.i[!(vect.spp.i$IDVal %in% vect.spp.i.t$IDVal), ]
vect.spp.i.novali2 <- terra::intersect(terra::vect(ocenamap), vect.spp.i.novali)
vect.spp.i.novali2 <- terra::intersect(terra::vect(oceanmap), vect.spp.i.novali)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Crees que esta parte se pueda hacer con el paquete sf sin tener que llamar a terra? algo asi como:

library(sf)
out <- st_intersection(points, poly)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Voy a actualizarlo para incluirla.

DESCRIPTION Show resolved Hide resolved
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cristian,
crees que sea posible aligerar los polígonos del mapa? y/o comprimirlo de alguna forma? Es para no superar 5 megas. Ya que como esta, con mas de 10 megas no nos deja subirlo a CRAN.

https://cran.r-project.org/web/packages/policies.html#:~:text=As%20a%20general%20rule%2C%20neither,to%20a%20maximum%20of%205MB.

Tal vez usando https://cran.r-project.org/web/packages/rmapshaper/vignettes/rmapshaper.html pueda ser una solucion...

Seria ideal usar un solo mapa para las dos funciones: mamm_coords_validator.R y mammalmap.R Viendo este problema tenemos que ser creativos para poder usar el mapa de municipios... tal vez hacer que el mapa se baje de alguna forma de GDAM con https://github.com/rspatial/geodata/ ?

Copy link
Collaborator Author

@crcruzr crcruzr Aug 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Si se pueden. La primer versón contó con la capa que está por defecto en el paquete, pero esta tiene muchos errores topológicos y arrojaba zonas sin información, a pesar de que los datos estaban claramente en un departmento.

Voy a incluir una de esas y agrego en comentarios los errores de precisión, para que un usuario que se cruce con este problema no piense que es un error del paquete.

… file for evaluation, and remove colmap_igac to reduce package size
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants