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.

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

Refactor and NA handling for 'decode()' #35

Merged
merged 4 commits into from
Sep 27, 2018

Conversation

ChrisMuir
Copy link
Contributor

@ChrisMuir ChrisMuir commented Sep 23, 2018

The first commit is in reference to issue #34, initial commit to allow decode() to handle NA input. Also added a test to test-Decode.R.

Previous code:

polylines <- c(
  "ohlbDnbmhN~suq@am{tAw`qsAeyhGvkz`@fge}A",
  NA_character_, 
  "ggmnDt}wmLgc`DesuQvvrLofdDorqGtzzV"
)

googlePolylines::decode(polylines)
#> [[1]]
#>      lat       lon
#> 1 26.774 -80.18999
#> 2 18.466 -66.11799
#> 3 32.321 -64.75700
#> 4 26.774 -80.18999

#> [[2]]
#>   lat lon
#> 1 -8e-05 1e-05

#> [[3]]
#>      lat       lon
#> 1 28.745 -70.57899
#> 2 29.570 -67.51400
#> 3 27.339 -66.66800
#> 4 28.745 -70.57899

Updated code:

polylines <- c(
  "ohlbDnbmhN~suq@am{tAw`qsAeyhGvkz`@fge}A",
  NA_character_, 
  "ggmnDt}wmLgc`DesuQvvrLofdDorqGtzzV"
)

googlePolylines::decode(polylines)
#> [[1]]
#>      lat       lon
#> 1 26.774 -80.18999
#> 2 18.466 -66.11799
#> 3 32.321 -64.75700
#> 4 26.774 -80.18999

#> [[2]]
#>   lat lon
#> 1  NA  NA

#> [[3]]
#>      lat       lon
#> 1 28.745 -70.57899
#> 2 29.570 -67.51400
#> 3 27.339 -66.66800
#> 4 28.745 -70.57899

For the encode() cases from issue #34, I'm happy to contribute PR's for them as well. I just really don't know enough about how mapdeck uses googlePolylines::encode() to suggest a best solution though.

@ChrisMuir
Copy link
Contributor Author

ChrisMuir commented Sep 23, 2018

The second commit contains edits to the Rcpp code related to decode(), refactoring for speed. All tests are passing locally. Changes include:

  • Return an Rcpp::List from function decode_polyline(), as opposed to an Rcpp::DataFrame. If you assign values to specific attributes of an Rcpp::List, it will "become" a data.frame on the R side, similar to the code/details in this Rcpp Gallary post. This is the source of most the speed up in the benchmarks below.
  • Create vectors pointsLat and pointsLon once, pass them by reference to func decode_polyline(), which calls .clear() on them to reuse them in each loop iteration. Originally, these vectors were being created everytime decode_polyline() was getting called.

Benchmarks

Test below is very simplistic and not robust, but the code edits yield a ~250x speed up.

library(googlePolylines)
library(microbenchmark)

# Create vector of 2000 polylines
polylines <- c(
  "ohlbDnbmhN~suq@am{tAw`qsAeyhGvkz`@fge}A",
  "ggmnDt}wmLgc`DesuQvvrLofdDorqGtzzV"
)
polylines <- rep(polylines, 1000)

Original code

microbenchmark::microbenchmark(
  decode(polylines), 
  times = 10
)
#> Unit: milliseconds
#>               expr      min       lq     mean   median       uq      max neval
#> decode(polylines) 473.4338 492.1889 497.8863 497.521 506.2926 517.1034    10

Updated code

microbenchmark::microbenchmark(
  decode(polylines), 
  times = 100
)
#> Unit: milliseconds
#>               expr      min       lq     mean   median       uq      max neval
#>  decode(polylines) 1.797718 1.868518 2.149002 1.921164 2.084554 5.080253   100

@ChrisMuir ChrisMuir changed the title handle NA inputs in 'decode()' Refactor and NA handling for 'decode()' Sep 23, 2018
@SymbolixAU
Copy link
Collaborator

great work @ChrisMuir , I'll review in the coming days, but it all looks good.

@ChrisMuir
Copy link
Contributor Author

👍 Cool sounds good. If you have any comments/questions, just let me know, I'm happy to make additional edits or tweaks to edits in my commits.

@SymbolixAU SymbolixAU merged commit 787b68a into SymbolixAU:master Sep 27, 2018
@SymbolixAU
Copy link
Collaborator

@ChrisMuir everything looks good - merged into master

SymbolixAU pushed a commit that referenced this pull request Sep 27, 2018
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.

2 participants