search

humaniverse / geographr

R package for mapping UK geographies
https://humaniverse.github.io/geographr/
Other
39 stars 7 forks source link

Addition of the Dec 2022 ward boundaries dataset #30

Closed jimgar closed 1 year ago

jimgar commented 1 year ago

Hi folks,

Hopefully a pretty straightforward PR here, just adding in the latest ward boundaries dataset.

This is my first attempted contribution to a package outside of my current job. A little milestone :)

Jim

MikeJohnPage commented 1 year ago

Hey Jim,

Thanks for the PR and good job!

Please can you make one minor amendment:

Your exported wards boundaries file is quite large (8.40 MB). Where possible, we want to simplify the polygons to reduce the file sizes to improve loading and computational speeds. While this does loose details on the boundaries, it is a compromise we are happy with in this package.

To simplify the 2022 wards polygons, you can make a call to rmapshaper::ms_simplify() before exporting the data set into the package and checking the file size. Note, that you will need to adjust the argument keep, as the standard setting of 0.05 will cause the dataset to lose rows. I've had a quick play around and setting keep = 0.61 seems to do the trick.

Thanks

MikeJohnPage commented 1 year ago

Also, I appreciate that we haven't been consistent in our requirement to simplify polygons in all the different data sets (e.g., only some do this), but this is an enhancement that we will try and do over time.

jimgar commented 1 year ago

Yep no problem, happy to make the change, Mike!

Thinking ahead, is it worth adding some text to the readme about checking polygon filesize and using rmapshaper::ms_simplify()?

I also have a GitHub question. There is now a conflict between branches for R/sysdata.rda. Is that something I will need to take care of on my side, and if so do you have any pointers?

MikeJohnPage commented 1 year ago

Thinking ahead, is it worth adding some text to the readme about checking polygon filesize and using rmapshaper::ms_simplify()?

Yes, that is not a bad idea if you would like to give it a try?

Is that something I will need to take care of on my side, and if so do you have any pointers?

No, this is something I will manage when I merge the branch into main. For now you can keep pushing commits to your fork. Read here for more information on this process.

jimgar commented 1 year ago

Hi Mike. I've simplified and re-exported the dataset, and added a note to the readme.

MikeJohnPage commented 1 year ago

Your changes have now been merged into main. Thank you for the contribution :grinning: