eblondel / cleangeo

Cleaning geometries from spatial objects in R
https://github.com/eblondel/cleangeo/wiki
44 stars 2 forks source link

Update clgeo_Clean.R #24

Closed kendonB closed 7 years ago

kendonB commented 7 years ago

Add pb to clgeo_Clean

eblondel commented 7 years ago

Hi @kendonB thanks for your suggestion, could you give some insight of this package? and how it would benefit (from your knowledge) to cleangeo. I see a benefit but for paralllelizing code rather than just adding the progress bar. In principle, i'd like to avoid adding extra dependencies where it could be avoided. Do you think we could leave it outside, e.g. by adding a"*apply" handler as argument instead?

kendonB commented 7 years ago

I really like pbapply simply because it gives pain-free progress bars. Since clgeo_Clean can take a long time, having some progress bar solution seems necessary. By ""*apply" handler as argument", do you mean allow the user to pass the looping function of their choice? That doesn't seem like a bad option but I'd still advise you to make it automatic, as in my solution, as this will help users who don't know about the relatively new pbapply, or aren't experts. This was the decision in the purrr package, for example: https://github.com/tidyverse/purrr/issues/149#issuecomment-200820240, and in dplyr::do.

You're also right that pbapply gives pain-free integration with the parallel package. This could also be quite easily integrated in my solution by adding a cl argument to clgeo_Clean.

Maybe @psolymos can comment on the stability of the API to alleviate your concerns about dependencies?

psolymos commented 7 years ago

@eblondel & @kendonB : I wrote the pbapply package as 'A lightweight package that adds progress bar to vectorized R functions' which now fully supports various parallel backends and listed in the HPC task view. It unifies the interface for different pb types available for an OS type and here is a post about how to integrate into other packages. Obviously, this is a dependency (I use it in multiple packages, that is why I factored out the functions), but I intend to keep it simple and not to build in other dependencies (something I do not like myself, so I understand your aversion :)

eblondel commented 7 years ago

Thanks @psolymos for this info. I've added pbapply as suggests. Thanks for @kendonB for suggesting this, indeed it is useful in cleangeo context