Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
14 changes: 14 additions & 0 deletions R/gh_fill.R
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
gh_fill <- function(geohashes, precision) {

@MichaelChirico MichaelChirico Jul 2, 2023

Copy link
Copy Markdown
Owner

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 = 1L

What should be the behavior here?

if (length(unique(nchar(geohashes))) > 1) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The 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 precision >= nchar(geohashes).

stop("Input Geohashes must all have the same precision level.")

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Is there no use case for taking e.g. c("0", "11") as input and wanting all of the level-4 precisions as output?

}
if (any(grepl("['ailoAILO]", geohashes))) {

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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 '?

stop("Invalid Geohash; Valid characters: [0123456789bcdefghjkmnpqrstuvwxyz](any case)")
}
new_levels <- precision - nchar(geohashes[1])
base32 <-

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I would cache this to .geohash at build time since it's likely to come up in various places:

.global = new.env(parent = emptyenv())

.geohash$base32 <- ...

unlist(strsplit("0123456789bcdefghjkmnpqrstuvwxyz", split = ""))
grid <-
do.call(data.table::CJ, append(list(geohashes), replicate(new_levels, base32, FALSE)))
do.call(paste0, grid)
}
65 changes: 65 additions & 0 deletions expandgrid_vs_CJ.Rmd
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))

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.



```

36 changes: 36 additions & 0 deletions man/gh_fill.Rd
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)
}
35 changes: 35 additions & 0 deletions tests/testthat/test-fill.R
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),

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

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)

})