Gh fill#41
Conversation
| "9ws", "9wt", "9wu", "9wv", "9ww", "9wx", | ||
| "9wy", "9wz")) | ||
| # Test vector of inputs | ||
| expect_equal(gh_fill(c("9w", "9x"), 3L), |
There was a problem hiding this comment.
I might expect the output here to be
list(
c("9w0", ..., "9wz"),
c("9x0", ..., "9xz")
)
That would make it much more natural to map inputs to outputs:
fill( x[j] ) == out[[jj]] instead of currently fill( x[j] ) == out[1:32 + (32 * (jj - 1L))]
WDYT?
We might also offer a simplify= (naming?) argument to toggle unlist() the version I propose.
| @@ -0,0 +1,14 @@ | |||
| gh_fill <- function(geohashes, precision) { | |||
| if (length(unique(nchar(geohashes))) > 1) { | |||
| stop("Input Geohashes must all have the same precision level.") | |||
There was a problem hiding this comment.
Is there no use case for taking e.g. c("0", "11") as input and wanting all of the level-4 precisions as output?
| stop("Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz](any case)") | ||
| } | ||
| new_levels <- precision - nchar(geohashes[1]) | ||
| base32 <- |
There was a problem hiding this comment.
I would cache this to .geohash at build time since it's likely to come up in various places:
Line 3 in 700fa97
.geohash$base32 <- ...
| if (length(unique(nchar(geohashes))) > 1) { | ||
| stop("Input Geohashes must all have the same precision level.") | ||
| } | ||
| if (any(grepl("['ailoAILO]", geohashes))) { |
There was a problem hiding this comment.
this allows non-ASCII characters on input, for example.
I would do if (!all(grepl(<base32>, geohashes))) instead, and use ignore.case=TRUE.
PS why '?
|
|
||
| ```{r benchmark test} | ||
| microbenchmark(gh_fill_dt(test_vector, 6L)) | ||
| microbenchmark(gh_fill_base(test_vector, 6L)) |
There was a problem hiding this comment.
On my machine the speedup is large but not massive (30%):
Unit: milliseconds
expr min lq mean median uq
gh_fill_dt(test_vector, 6L) 497.8866 723.1318 797.3614 793.1192 871.561
gh_fill_base(test_vector, 6L) 814.1263 1031.9389 1225.3436 1160.6757 1457.280
max neval
1176.892 100
1788.559 100
Anyway, data.table is not complex as far as dependencies go... I lean towards allowing it.
|
WDYT about designing this as
|
| @@ -0,0 +1,14 @@ | |||
| gh_fill <- function(geohashes, precision) { | |||
| if (length(unique(nchar(geohashes))) > 1) { | |||
There was a problem hiding this comment.
See overall comment... if we only want to allow "zooming in", we should add a check that precision >= nchar(geohashes).
| @@ -0,0 +1,14 @@ | |||
| gh_fill <- function(geohashes, precision) { | |||
There was a problem hiding this comment.
Please also consider some edge cases:
geohashes = character()
geohashes = NA_character_
geohashes = c("0", NA_character_)
geohashes = "0"; precision = 1L
What should be the behavior here?
2 participants
Made the following changes: