-
Notifications
You must be signed in to change notification settings - Fork 3
Gh fill #41
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.
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
base: master
Are you sure you want to change the base?
Gh fill #41
Changes from all commits
0169741
1335247
de4a3b0
07ec5b7
5c4c1cc
cfe7cf6
2e51c15
b3f5016
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,14 @@ | ||||
| gh_fill <- function(geohashes, precision) { | ||||
| if (length(unique(nchar(geohashes))) > 1) { | ||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See overall comment... if we only want to allow "zooming in", we should add a check that |
||||
| stop("Input Geohashes must all have the same precision level.") | ||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there no use case for taking e.g. |
||||
| } | ||||
| if (any(grepl("['ailoAILO]", geohashes))) { | ||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this allows non-ASCII characters on input, for example. I would do PS why |
||||
| stop("Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz](any case)") | ||||
| } | ||||
| new_levels <- precision - nchar(geohashes[1]) | ||||
| base32 <- | ||||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would cache this to Line 3 in 700fa97
|
||||
| unlist(strsplit("0123456789bcdefghjkmnpqrstuvwxyz", split = "")) | ||||
| grid <- | ||||
| do.call(data.table::CJ, append(list(geohashes), replicate(new_levels, base32, FALSE))) | ||||
| do.call(paste0, grid) | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| --- | ||
| title: "Justification for data.table import" | ||
| output: html_document | ||
| date: "2023-06-30" | ||
| --- | ||
|
|
||
| ```{r Define both functions} | ||
| library(data.table) | ||
| library(microbenchmark) | ||
|
|
||
| gh_fill_dt <- function(geohashes, precision) { | ||
| if (uniqueN(nchar(geohashes)) > 1) { | ||
| stop("Input Geohashes must all have the same precision level.") | ||
| } | ||
| if (sum(grepl("['ailo]", geohashes)) > 0) { | ||
| stop("Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz]") | ||
| } | ||
|
|
||
| new_levels <- precision - nchar(geohashes[1]) | ||
|
|
||
| base32 <- | ||
| unlist(strsplit("0123456789bcdefghjkmnpqrstuvwxyz", split = "")) | ||
|
|
||
| grid <- | ||
| do.call(data.table::CJ, append(list(geohashes), replicate(new_levels, base32, FALSE))) | ||
|
|
||
| do.call(paste0, grid) | ||
|
|
||
| } | ||
|
|
||
|
|
||
| gh_fill_base <- function(geohashes, precision) { | ||
| if (uniqueN(nchar(geohashes)) > 1) { | ||
| stop("Input Geohashes must all have the same precision level.") | ||
| } | ||
| if (sum(grepl("['ailo]", geohashes)) > 0) { | ||
| stop("Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz]") | ||
| } | ||
|
|
||
| new_levels <- precision - nchar(geohashes[1]) | ||
|
|
||
| base32 <- | ||
| unlist(strsplit("0123456789bcdefghjkmnpqrstuvwxyz", split = "")) | ||
|
|
||
| grid <- | ||
| do.call(expand.grid, append(list(geohashes), replicate(new_levels, base32, FALSE))) | ||
|
|
||
| do.call(paste0, grid) | ||
|
|
||
| } | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| ```{r create test vector} | ||
| test_vector <- gh_fill_dt(c("9w", "9x"), 4L) # Length of 2048 | ||
| ``` | ||
|
|
||
| ```{r benchmark test} | ||
| microbenchmark(gh_fill_dt(test_vector, 6L)) | ||
| microbenchmark(gh_fill_base(test_vector, 6L)) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On my machine the speedup is large but not massive (30%): Anyway, data.table is not complex as far as dependencies go... I lean towards allowing it. |
||
|
|
||
|
|
||
| ``` | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| \name{gh_fill} | ||
| \alias{gh_fill} | ||
|
|
||
| \title{ | ||
| Fill geohash(s) | ||
| } | ||
| \description{ | ||
| Fill geohash prefix with members | ||
| } | ||
| \usage{ | ||
| gh_fill(geohashes, precision) | ||
| } | ||
| \arguments{ | ||
| \item{geohashes}{ Character vector of input geohashes. They must all be of same precision. } | ||
| \item{precision}{ Positive \code{integer} scalar controlling the 'zoom level' -- how many characters should be used in the output. } | ||
| } | ||
| \details{ | ||
| Note that each increas of \code{precision} by 1 results in an output 32 times larger. | ||
| } | ||
| \value{ | ||
| \code{character} vector of geohashes corresponding to the input. | ||
| } | ||
| \references{ | ||
| \url{http://geohash.org/} ( Gustavo Niemeyer's original geohash service ) | ||
| } | ||
| \author{ | ||
| Garnet vanWeerden | ||
| } | ||
|
|
||
| \examples{ | ||
| # Single geohash input | ||
| gh_fill("9w", 6L) | ||
|
|
||
| # Vector of geohashes of same length. | ||
| gh_fill(c("9w", "9x", "9y"), 6L) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| test_that('gh_fill works', { | ||
|
|
||
| # Test single string | ||
| expect_equal(gh_fill("9w", 3L), c("9w0", "9w1", "9w2", "9w3", "9w4", "9w5", | ||
| "9w6", "9w7", "9w8", "9w9", "9wb", "9wc", | ||
| "9wd", "9we", "9wf", "9wg", "9wh", "9wj", | ||
| "9wk", "9wm", "9wn", "9wp", "9wq", "9wr", | ||
| "9ws", "9wt", "9wu", "9wv", "9ww", "9wx", | ||
| "9wy", "9wz")) | ||
| # Test vector of inputs | ||
| expect_equal(gh_fill(c("9w", "9x"), 3L), | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might expect the output here to be That would make it much more natural to map inputs to outputs:
WDYT? We might also offer a |
||
| c("9w0", "9w1", "9w2", "9w3", "9w4", "9w5", | ||
| "9w6", "9w7", "9w8", "9w9", "9wb", "9wc", | ||
| "9wd", "9we", "9wf", "9wg", "9wh", "9wj", | ||
| "9wk", "9wm", "9wn", "9wp", "9wq", "9wr", | ||
| "9ws", "9wt", "9wu", "9wv", "9ww", "9wx", | ||
| "9wy", "9wz", | ||
| "9x0", "9x1", "9x2", "9x3", "9x4", "9x5", | ||
| "9x6", "9x7", "9x8", "9x9", "9xb", "9xc", | ||
| "9xd", "9xe", "9xf", "9xg", "9xh", "9xj", | ||
| "9xk", "9xm", "9xn", "9xp", "9xq", "9xr", | ||
| "9xs", "9xt", "9xu", "9xv", "9xw", "9xx", | ||
| "9xy", "9xz")) | ||
|
|
||
| # Going down level by level should have same result as doing it all at once | ||
| expect_equal(gh_fill("9w", 4L), | ||
| gh_fill( | ||
| gh_fill("9w", 3L), | ||
| 4L) | ||
| ) | ||
|
|
||
| # Only Valid Characters | ||
| expect_error(gh_fill("i9", 3L), 'Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz](any case)', fixed = TRUE) | ||
|
|
||
| }) | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also consider some edge cases:
geohashes = character()geohashes = NA_character_geohashes = c("0", NA_character_)geohashes = "0"; precision = 1LWhat should be the behavior here?