jbaileyh / geogrid

Turning geospatial polygons into regular or hexagonal grids. For other similar functionality see the tilemaps package https://github.com/kaerosen/tilemaps
Other
393 stars 31 forks source link

Get CRAN-ready #17

Closed hafen closed 6 years ago

hafen commented 6 years ago

There are several things to address with R CMD check to get this package ready for release on CRAN. I'm happy to do all of these things and issue a PR.

Regardless of CRAN release, it would still be good to have these updates in place so that other packages can depend on this package without issue. If you are okay with me working on these, let me know and I'll get started.

sassalley commented 6 years ago

@hafen that would be fantastic thank you. This also raises a larger question around the value of having this as a stand alone package. Conceptually, the package only really provides two functions.

  1. Generating grids (regular or hexagonal) that are similar to the existing geography.
  2. Assigning existing geographies to the new grid.

Given your experience with submitting to CRAN do you think this is sufficient functionality for a stand-alone package or would it make more sense to add it as a feature to geofacet? I would be happy for the function to be incorporated into geofacet and believe that it would be well suited there. Of course - it would be your decision but just wondered your thoughts?

Best, Joe

hafen commented 6 years ago

Great! I've done some work today along these lines. I'll submit a PR with notes of what's been done. I have some questions there for you to address.

I think hexmapr is completely sufficient to be its own package. I see it having a great deal of functionality beyond geofacet, such as providing hexagonal grids which geofacet does not make use of and other packages could. So I'd vote for pushing this out there as standalone. I already have some code in geofacet in place to import from this package, hence the motivation on my end to help put hexmapr on CRAN :).

Nowosad commented 6 years ago

Hey @jbaileyh, can you let me know when do you plan to update the CRAN version of the geogrid package?

jbaileyh commented 6 years ago

Hi @Nowosad thanks for asking. Does the timeline have anything to do with geocompr? At the moment, I think the build version is still passing but I wont be available to update for around a month. If i recall correctly, @hafen was planning on integrating this (or similar) functionality into geofacet package but i'm not sure if this is still the case?

hafen commented 6 years ago

@jbaileyh - geofacet depends on geogrid and makes direct calls to it, so the packages are integrated in that way, but I don't plan on moving any geogrid functionality directly into geofacet.

I've been a little unplugged from things lately - are there major changes in geogrid I should be aware of?

Nowosad commented 6 years ago

I think the only change is added support for the sf objects. @jbaileyh, we use geogrid (together with sf) in one of the exercises. It would be nice to have it on CRAN. Let me know if you need any help with it.

jbaileyh commented 6 years ago

In the shorter term I think all that would be needed is to ensure that assign_polygons() returns sf objects. Would you suggest that this is true? I think the only thing that we'd need to do is use st_as_sf() on the output.

I won't be able to make this change super quickly but if you're happy to do so, i'll submit the update to CRAN if that would be welcome?

Nowosad commented 6 years ago

It is already here - https://github.com/jbaileyh/geogrid/blob/cb7e742758a066cc74bdd634664faaaec64c693b/R/assign_polygons.R#L103.

jbaileyh commented 6 years ago

Apologies - I forgot about this helpful addition. @Nowosad does the example in the readme work for you when using calculate_grid.sf and assign_polygons.sf?

Nowosad commented 6 years ago

Hey @jbaileyh ! My plan for today is to clean the readme file a bit and next to test its examples using sf objects. I will keep you up to date with my progress.