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.

Sign up for GitHub

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

Jump to bottom

Added gh_fill.R #35

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Added gh_fill.R #35

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e96587
Added gh_fill via upload
gvanweerden Aug 9, 2022
674da9d
Merge pull request #1 from gvanweerden/gvanweerden-gh_fill-1
gvanweerden Aug 9, 2022
f7272d8
Merge branch 'MichaelChirico:master' into master
gvanweerden Jun 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions R/gh_fill.R
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#' Fill geohash prefix with members
Copy link
Owner

Choose a reason for hiding this comment

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

The package isn't currently using roxygen2, so you'll need to either write the .Rd directly, or convert the rest of the package to use roxygen2.

#'
#' @param geohashes Character vector of input geohashes. They must all be of same precision
#' @param precision Positive integer scalar controlling the 'zoom level' – how many characters should be used in the output.
#' @return Character vector of geohashes corresponding to the input.
#' @export
Copy link
Owner

Choose a reason for hiding this comment

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

please add examples and tests, it will especially make it easier to review when I can see expected inputs/outputs




gh_fill <- function(geohashes, precision) {
if (uniqueN(nchar(geohashes)) > 1) {
Copy link
Owner

Choose a reason for hiding this comment

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

uniqueN is a data.table function, which we don't import. length(unique()) should be fine here.

stop("Input Geohashes must all have the same precision level.")
}
if (sum(grepl("['ailo]", geohashes)) > 0) {
Copy link
Owner

Choose a reason for hiding this comment

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

prefer any(grepl()).

also, elsewhere in the package, we allow upper-case input, we should strive to match case here.

stop("Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz]")
}

new_levels <- precision - nchar(geohashes[1])

base32 <-
Copy link
Owner

Choose a reason for hiding this comment

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

it's probably better to run this at compile-time in the package environment, rather than every time gh_fill() is invoked

unlist(strsplit("0123456789bcdefghjkmnpqrstuvwxyz", split = ""))

grid <-
do.call(data.table::CJ, append(list(geohashes), replicate(new_levels, base32, FALSE)))
Copy link
Owner

Choose a reason for hiding this comment

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

could you do a performance comparison of CJ() vs. expand.grid() for typical inputs? E.g. 1e4 inputs and adding 1-3 characters of precision...

Depending on that we'll add the data.table dependency


do.call(paste0, grid)

}