Skip to content

Comments

Coleman dev#37

Open
aecoleman wants to merge 7 commits intojbaileyh:masterfrom
aecoleman:coleman-dev
Open

Coleman dev#37
aecoleman wants to merge 7 commits intojbaileyh:masterfrom
aecoleman:coleman-dev

Conversation

@aecoleman
Copy link

Was able to remove the for loop in assign_polygons, which should make it much more efficient for larger datasets. Also transitioned assign_polygons to use sf primarily, and convert sp objects to sf (and back) when they are supplied.

I also fixed some lints (that probably weren't lints too long ago.

Finally, I added a function for generating samples of contiguous polygons from a larger sf object. I found it useful for benchmarking a larger geogrid job (hexgrid of all 3000+ CONUS Counties).

@jbaileyh
Copy link
Owner

Thanks for submitting! Will take a look as soon as possible.

@aecoleman
Copy link
Author

Let me know if you have any questions, comments or suggestions. I hope I didn't break the lintr setup. Oh, and I just realized that I forgot to run it through R CMD Check, so it would be a good idea to make sure I didn't introduce any new issues there.

@conorotompkins
Copy link

Was this commit merged? I am running into issues with the exact task that @aecoleman mentioned ("hexgrid of all 3000+ CONUS Counties"). The package works very well, but it would be fantastic if it could handle larger jobs.

@aecoleman
Copy link
Author

@conorotompkins I don't believe it has been merged. You can install the version on my github and see how that works for you. The command to do that would be remotes::install_github("aecoleman/geogrid", ref = "coleman-dev").

Note that I have not tested this since R 4.0.0 came out, so YMMV.

Also, when I did this, I had some issues with hierarchical integrity. Counter to my expectations, states would end up scattered about. Squaring the cost matrix might alleviate this, but I haven't had the chance to try it out, yet. (I mentioned how long it took to run.) I've also since seen faster implementations of the cost minimization algorithm, which I haven't had a chance to play around with (the clue package contains one, I think).

@jbaileyh
Copy link
Owner

@conorotompkins @aecoleman apologies - this hasn't been merged. I was going to suggest installing the branch kindly submitted by @aecoleman in the medium term.

@hafen hafen mentioned this pull request Apr 27, 2023
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.

3 participants