ATFutures / geoplumber

Serve geographic data from R and consume with scalable front end.
https://atfutures.github.io/geoplumber/
59 stars 7 forks source link

example app fails #57

Closed silberzwiebel closed 5 years ago

silberzwiebel commented 5 years ago

Hey there :)

I was following these steps from the README to get geoplumber up and running:

sudo npm i -g create-react-app
R
> library(geoplumber)
> gp_create("my_app")
> setwd("my_app")
> gp_build()
> gp_plumb()

However, gp_plumb() crashes R completely (the output of the other commands read fine too me):

> gp_plumb()

 *** caught segfault ***
address 0x2000002d, cause 'memory not mapped'

Traceback:
 1: rcpp_sf_to_geojson(sf, digits, factors_as_string)
 2: sf_geojson.sf(uol)
 3: geojsonsf::sf_geojson(uol)
 4: eval(ei, envir)
 5: eval(ei, envir)
 6: withVisible(eval(ei, envir))
 7: source(file, local = private$envir, echo = FALSE, keep.source = TRUE)
 8: .subset2(public_bind_env, "initialize")(...)
 9: plumber$new(file)
10: plumber::plumb(file = file)
11: gp_plumb()

Possible actions:
1: abort (with core dump, if enabled)
2: normal R exit
3: exit R without saving workspace
4: exit R saving workspace
Selection: 

I did this on two different machines, both running Fedora 29. I tested with both the node version from Fedora's repository (version 10.15.0) and also once with node installed from here (version 11.6.0). Am I missing something obvious (I'm very much new to JS development in total)?

layik commented 5 years ago

Hey @silberzwiebel thanks for reporting this! Get back to you soon.

layik commented 5 years ago

I can reproduce that and yes, from your traceback it looks like geojsonsf::sf_geojson(uol) to blame. Will fix and reference this issue @silberzwiebel. Apologies, I can only blame the dependencies and lack of test code for 100% coverage.

layik commented 5 years ago

OK we blamed the right line and @SymbolixAU might know what has changed.

To fix:

my_app/R/plumber.R line 35 should be

uol <- geojsonsf::sf_geojson(uol)

please change it to uol <- geojsonsf::sf_geojson(uol, factors_as_string=FALSE)

That should make geojsonsf happy.

I will push the change now and I think, you just fixed my Travis, too.

The other way of course, is once you pull the latest commit, create a fresh app but I would advice against this as you already have my_app.

layik commented 5 years ago

Did not mean to close it, please close it when your code runs fine.

layik commented 5 years ago

Thanks @silberzwiebel one more time, this fixed Travis too.

SymbolixAU commented 5 years ago

It's odd that it crashed; I made the new function have defaults values, so I don't know why this happened.


sf_geojson <- function( sf, atomise = FALSE, simplify = TRUE, digits = NULL, factors_as_string = TRUE ) {
   ...
}

I've made an issue to look into this.


Update:

it looks like the issue is where all factor levels are the same

SymbolixAU commented 5 years ago

Update:

I've tracked down the culprit and made a few changes https://github.com/SymbolixAU/geojsonsf/issues/62

Thanks for letting me know about this.

layik commented 5 years ago

Thank you @SymbolixAU for your immediate reply/changes. I have now setup a cron job on Travis which should grab such changes.

silberzwiebel commented 5 years ago

Wow, thanks a lot for the quick responses and fixes! I didn't have an actual app yet, so it was fine for me to just start it all over again and not it works out-of-the-box. Thanks!

SymbolixAU commented 5 years ago

As a side note, I made factors_as_string = TRUE a default option in the latest release to fit in with the more modern r-paradigms, where factors are becoming less and less important. For example, the tidyverse sets stringsAsFactors = FALSE by default, and so does sf I think.

Plus, I think it makes more sense, when converting to GeoJSON, to use the string representation of a factor, rather than the number

df <- data.frame(val = c("a","b","c"))
df
# val
# 1   a
# 2   b
# 3   c

jsonify::to_json( df )
'[{"val":"a"},{"val":"b"},{"val":"c"}]'

## vs

jsonify::to_json( df, factors_as_string = FALSE )
'[{"val":1},{"val":2},{"val":3}]'
layik commented 5 years ago

Thanks @SymbolixAU. I do not count myself an expert here both in serialising JSON or the R strings_as_factors in R. Just notice that when

library(geoplumber)
uol <- rbind(uni_point, uni_poly)
unlist(lapply(uol, class))
#>      osm_id        name     amenity        note   geometry1   geometry2 
#>    "factor"    "factor"    "factor"    "factor" "sfc_POINT"       "sfc"

Created on 2019-01-20 by the reprex package (v0.2.1)

Not sure which factor column was being converted to strings. Once I am a bit more R'literate, I may perhaps contribute constructively.

SymbolixAU commented 5 years ago

Hopefully this example explains it a bit better

library(geoplumber)
library(geojsonsf)

uol <- rbind(uni_point, uni_poly )

This is the original behaviour where factors were treated as-is. Notice the properties are numbers (i.e. the factors).

fac <- sf_geojson( uol , factors_as_string = FALSE )
substr( as.character( fac ), 1, 200 )
# [1] "{\"type\":\"FeatureCollection\",\"features\":[{\"type\":\"Feature\",\"properties\":{\"osm_id\":1,\"name\":1,\"amenity\":1,\"note\":1},\"geometry\":{\"type\":\"Point\",\"coordinates\":[-1.556264346395547,53.806309971633918]}},{\"t"

Whereas in this new version the properties are now strings.

char <- sf_geojson( uol, factors_as_string = TRUE )
substr( as.character( char ), 1, 200 )
# [1] "{\"type\":\"FeatureCollection\",\"features\":[{\"type\":\"Feature\",\"properties\":{\"osm_id\":\"84656164\",\"name\":\"University of Leeds\",\"amenity\":\"university\",\"note\":\"Not all owned by uni, this area is misleading\"},"