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

Improve pointlist output for isochrone (/spt endpoint) #1577

Merged
merged 21 commits into from
Jun 28, 2019
Merged

Conversation

karussell
Copy link
Member

@karussell karussell commented Mar 26, 2019

Update: this is now a pure CSV endpoint:

longitude,latitude,time,distance
lon1, lat1, time1, distance1
lon2, lat2, time2, distance2

where you can add more columns like prev_latitude,prev_longitude or path details like road_class

The client code example snippet is here and also see the deck.gl for R tool.

Original text:

The pointlist option was not really useful as it came with just the coordinates. Have now created a compact but extendable JSON format (read update).

{  "description": ["longitude", "latitude", "time", "distance"],
   "items": [
     [lng, lat, time, distance],
     [...]
   ]
} 

Thx @crazycapivara for the valuable discussion! He created a deck.gl for R tool and future versions will likely work with graphhopper. See also the work here with deck.gl and a forked graphhopper (this should be possible after this PR without forking). See also related #1572.

TODOs:

  • use different endpoint name, e.g. /shortest-path-tree or /spt
  • create List<IsoLabelWithCoordinates> in IsochroneResource to make Isochrone method more powerful?
  • allow paging through the pointlist result with some caching? later

@karussell karussell changed the title Improve pointlist for isochrone Improve pointlist output for isochrone Mar 26, 2019
@crazycapivara
Copy link

Here is an example on how to visualize data with deckgl and R

library(magrittr)
library(jsonlite)
library(deckgl)

lat <- 51.33399
lng <- 9.438801

# Get data
apiCall <- paste0(
  "http://localhost:8989/isochrone?point=", lat, ",", lng,
  "&time_limit=600&result=pointlist&pointlist_ext_header=prev_longitude,prev_latitude"
)

resp <- httr::GET(apiCall) %>%
  httr::content("text") %>%
  fromJSON(flatten = TRUE)

# Prepare data
tbl <- resp$items %>%
  set_colnames(resp$header) %>%
  tibble::as_tibble() %>%
  subset(time != 0)

# Plot data
properties <- list(
  getStrokeWidth = 3,
  getSourcePosition = get_position("prev_latitude", "prev_longitude"),
  getTargetPosition = get_position("latitude", "longitude"),
  getTooltip = get_property("distance")
)

# Create deckgl widget
deck <- deckgl(latitude = lat, longitude = lng, pickingRadius = 2)

deck_black <- deckgl(
  latitude = lat,
  longitude = lng,
  pickingRadius = 2,
  style = list(background = "black")
)

# Arc layer
deck %>%
  add_arc_layer(data = tbl, properties = properties)

# Create colors for line layer
pal <- leaflet::colorBin(tbl$distance)
tbl$color <- pal(tbl$distance)

# Line layer
deck_black %>%
  add_line_layer(
    data = tbl, properties = properties,
    getColor = get_color_to_rgb_array("color")
  )

@karussell
Copy link
Member Author

Thanks a lot!

subset(time != 0)

Here it says something about the type:

Fehler in time != 0 : Vergleich (2) ist nur für atomare und Listentypen möglich

@karussell
Copy link
Member Author

Ups, using a different starting point fixed this for me. E.g. for Berlin:

lat <- 52.509535
lng <- 13.40332

@karussell
Copy link
Member Author

Nice, got some streetnetwork as output -> looks funny those curved streets. Thanks @crazycapivara!

For the leaflet part I had to do:

install.packages("leaflet")
library(leaflet)

But then I get:

Fehler in leaflet::colorBin(tbl$distance) :
Argument "domain" fehlt (ohne Standardwert)

@crazycapivara
Copy link

crazycapivara commented Mar 28, 2019

Sorry, I posted an old version of the script, will send a working update.

@crazycapivara
Copy link

Start a container with Berlin data set:

docker run -p 8989:8989 -d --name gh-berlin crazycapivara/graphhopper:ext_pointlist 

Run some visualizations wit R and deckgl:

library(magrittr)
library(jsonlite)
library(deckgl)

lat <- 52.517057
lng <- 13.3992

# Get data
apiCall <- paste0(
  "http://localhost:8989/isochrone?point=", lat, ",", lng,
  "&time_limit=600&result=pointlist&pointlist_ext_header=prev_longitude,prev_latitude"
)

resp <- httr::GET(apiCall) %>%
  httr::content("text") %>%
  fromJSON(flatten = TRUE)

# Prepare data
tbl <- resp$items %>%
  set_colnames(resp$header) %>%
  tibble::as_tibble() %>%
  subset(time != 0)

# Plot data
properties <- list(
  getStrokeWidth = 3,
  getSourcePosition = get_position("prev_latitude", "prev_longitude"),
  getTargetPosition = get_position("latitude", "longitude"),
  getTooltip = get_property("distance")
)

# Create deckgl widget
deck <- deckgl(latitude = lat, longitude = lng, pickingRadius = 2)

# Arc layer
deck %>%
  add_arc_layer(data = tbl, properties = properties)

# Create colors for line layer
pal <- leaflet::colorBin("RdYlBu", tbl$distance)
tbl$color <- pal(tbl$distance)

# Create a deckgl widget with black background
deck_black <- deckgl(
  latitude = lat,
  longitude = lng,
  pickingRadius = 2,
  style = list(background = "black")
)

# Line layer
deck_black %>%
  add_line_layer(
    data = tbl, properties = properties,
    getColor = get_color_to_rgb_array("color")
  )

# Arc and line layer
deck %>%
  add_data(tbl) %>%
  add_arc_layer(data = get_data(), properties = properties) %>%
  add_line_layer(
    data = get_data(), properties = properties,
    getColor = get_color_to_rgb_array("color")
  )

If you have valid mapbox api key you can add a basemap as well:

deck %>%
   ... %>%
  add_mapbox_basemap(api_key)

@crazycapivara
Copy link

graphhopper-deckgl-R

@crazycapivara
Copy link

Maybe we could add an offset parameter to the query to avoid too large data sets to be transferred at once, e. g.:

...?time_limit=3600&offset=3000

would return points where time_limit is between 3000 and 3600.

Furthermore, at the moment, the request returns a lot of points above the given time_limit.

@karussell
Copy link
Member Author

would return points where time_limit is between 3000 and 3600

Why would this help? To print parts of the result before everything arrived? The problem is that we would then have to remember the state somehow (cache) which I would like to avoid if possible.

Furthermore, at the moment, the request returns a lot of points above the given time_limit.

Ah, yes. This is ugly and I can fix it here: https://github.com/graphhopper/graphhopper/blob/master/isochrone/src/main/java/com/graphhopper/isochrone/algorithm/Isochrone.java#L94

@crazycapivara
Copy link

Why would this help? To print parts of the result before everything arrived? The problem is that we would then have to remember the state somehow (cache) which I would like to avoid if possible.

Yes, this was one idea and to maybe allow larger time_limits but I can unsterstand that you want to avoid caching.

@karussell
Copy link
Member Author

Hmmh ... if we allow caching then we could return the isochrone-shortest-path-tree via the MVT endpoint (#1572), which would save us from a non-standard format.

@crazycapivara
Copy link

Ups, Nope, I would like to keep the format. It is perfect for a lot of use cases. I just connected the API to a postgis container, so that you can query the roads with SELECT way, time, distance FROM graphhopper.way(lat, lng, time_limit). Furthermore, deck.gl supports the format as well.

@crazycapivara
Copy link

see docker-postgis-graphhopper

@karussell
Copy link
Member Author

Ah, ok. Thanks for the feedback!

It is perfect for a lot of use cases. I just connected the API to a postgis container, so that you can query the roads with

This sounds great!

@michaz
Copy link
Member

michaz commented Apr 10, 2019

The pointlist option was not really useful as it came with just the coordinates. Have now created a compact but extendable JSON format:

@karussell Did you just invent CSV-over-JSON? ,-)

@karussell
Copy link
Member Author

Lol. I'm pretty sure this is already existent :D ... how do you like it ;) ?

@michaz
Copy link
Member

michaz commented Apr 11, 2019

I like it! But maybe consider using CSV right away for this endpoint variant of the endpoint?

  • It's exactly like CSV. Variable number of columns, string-indexed, non-hierarchical records.
  • CSV is stream-friendly. You can parse JSON as a stream, but a JSON output still has this "I am a document, not a stream of lines, wait for the closing bracket before you start interpreting me" feel to it (like XML).
  • Some people invented the opposite, JSON-over-CSV, for when they have hierarchical data, so CSV still seems to have some merit.. http://csvjson.org
  • In client code, a CSV library would directly know what you mean when you say e.g. line.getColumnValue("distance"), but for the JSON version, you would have to implement that yourself in most languages (not in R, apparently).

@crazycapivara Any thoughts?

(No need to decide, since this can be easily added as a second mediatype on the same endpoint. But just in case you're immediately convinced, we could switch.)

@crazycapivara
Copy link

For now I would like to add it as a second mediatype and compare both endpoints for my use cases.

@karussell
Copy link
Member Author

karussell commented Apr 12, 2019

case "latitude":
list.add(label.adjCoordinate.y);
break;
case "prev_longitude":
Copy link

Choose a reason for hiding this comment

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

Can we please add options to export previous node's time and distance, so we can get values in both ends of segment without a lookup join?

                        case "prev_distance":
                            list.add(Optional.ofNullable(index.get(label.baseNodeId)).map(i -> i.distance).orElse(0D));
                            break;
                        case "prev_time":
                            list.add(Optional.ofNullable(index.get(label.baseNodeId)).map(i -> i.time).orElse(0L));
                            break;

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have an example why this would be useful for you?

Copy link

Choose a reason for hiding this comment

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

Yeah. Builiding a constrained Delaunay triangulation and clipping it in PostGIS, I need to have two-point segments to feed into function. Here's more detail: https://www.patreon.com/posts/isochrones-are-20933638

@Komzpa
Copy link

Komzpa commented Apr 18, 2019

This thing is useful for us. Format is fine, the only thing is - can we have travel times and distances in both ends of a segment, and export as such? Snippet attached.

… distance with test; changed unit of time to seconds to save some bandwidth
@karussell
Copy link
Member Author

Have added it. Please note that I have changed the time to seconds to reduce bandwidth a bit and ms does not make much sense - or is there a good reason to keep milliseconds?

@karussell
Copy link
Member Author

karussell commented May 31, 2019

Ok, have now converted this endpoint into returning CSV. Additionally as dropwizard makes this very easy, the CSV is already streamed, i.e. even for large shortest path trees the client gets something to do right after the first nodes are traversed :)

@karussell
Copy link
Member Author

karussell commented May 31, 2019

Oh, this means we can already fetch country-sized (or even continental-sized) shortest path trees without any memory problems. The client just waits a bit for the data transmission and sees the exploration in (near) real time.

@karussell
Copy link
Member Author

karussell commented Jun 2, 2019

Country-sized SPTs are definitely doable with this endpoint:

image

The JavaScript code snippet is here.
The data for time_limit=2h is not small (140MB; gzipped=67MB) but the response time is ok (< 10sec) and there is a good solution for streamed CSV parsing for the browser or node that allows you to do the downloading in a separate worker.

But with leaflet (preferCanvas:true or renderer:L.canvas()) I was not able to get updates while downloading and also everything was very slow (updates takes ages for loading and zooming).

Would be curious if the deck.gl for R work from @crazycapivara is able to handle that somehow :)

@michaz
Copy link
Member

michaz commented Jun 3, 2019

Nice!

But with leaflet (preferCanvas:true or renderer:L.canvas()) I was not able to get updates while downloading and also everything was very slow (updates takes ages for loading and zooming).

Yes. I think this would also overwhelm e.g. QGis. Layers with that many objects are a new thing.

@crazycapivara
Copy link

lat <- 52.321911
lng <- 10.393066
time_limit <- 7200

api_call <- paste0(
  "http://localhost:8989/spt?point=",
  lat, ",", lng,
  "&time_limit=", time_limit,
  "&columns=prev_longitude,prev_latitude,longitude,latitude,time"
)

dt <- data.table::fread(api_call, na.strings = "null") %>%
  na.omit()

pal <- leaflet::colorBin("RdYlBu", dt$time)
dt$color <- pal(dt$time)

library(deckgl)

deckgl(
  latitude = lat,
  longitude = lng,
  style = list(background = "black")
) %>%
  add_line_layer(
    data = dt,
    getStrokeWidth = 3,
    getSourcePosition = get_position("prev_latitude", "prev_longitude"),
    getTargetPosition = get_position("latitude", "longitude"),
    getTooltip = get_property("time"),
    getColor = get_color_to_rgb_array("color")
  )

In this case it takes some time until the layer is rendered because first all data is downloaded and then passed to the widget (from R to json). But in the end when it shows up zooming is smooth.

I need to add a csv-reader on javascript side to speed up loading.

@crazycapivara
Copy link

crazycapivara commented Jun 7, 2019

gh-spt

Added a pure JavaScript snippet here where you can click on the map to set the center point and a drop down list to select the time limit using d3js to read the CSV and deck.gl to render the lines.

@michaz
Copy link
Member

michaz commented Jun 7, 2019

Love it!

@karussell What do you think about this (from above):

The user specifies all the columns they want, not only those in addition to "longitude", "latitude", "time", "distance". Just let "longitude", "latitude", "time", "distance" be the default.

Thus, all columns are treated equal. If I want only "latitude", "longitude", "distance", I say columns=latitude,longitude,distance.

I think that would make it more flexible (can also drop columns) and easier do document: "Specify the columns you want" instead of "specify the columns in addition to the default columns that you want".

And I think two fewer lines of code. ;-)

@karussell
Copy link
Member Author

I think that would make it more flexible (can also drop columns) and easier do document

Yes, this was a good idea! And if I did not misunderstand it, then this is already done :D ?

BTW: in some days or weeks we'll have #1548 i.e. information like road_class, max_speed, tunnel, toll etc then we can make this available here too. Furthermore we can allow to specify a filter if only main roads are required (or a smaller+faster output) or visualizing tunnels and bridges is required for risk analysis etc

@crazycapivara
Copy link

@karussell That sounds great, then we can also use this information for the styling.

@karussell
Copy link
Member Author

karussell commented Jun 15, 2019

You can now request additional information like road_class (highway tag) and road_environment (tunnel, bridge, ...) and more. Do the following steps:

  • update to latest version
  • use the following param in config.yml: graph.encoded_values: surface,road_class,road_environment,road_access,max_speed
  • do a re-import
  • specify them as additional columns in the request

@karussell
Copy link
Member Author

@michaz do you want to review this again? Especially as you were asking:

The user specifies all the columns they want, not only those in addition to "longitude", "latitude", "time", "distance".

@karussell
Copy link
Member Author

If no opposing opinion I'll merge this today

@Komzpa
Copy link

Komzpa commented Jun 28, 2019

I haven't run it but reading through description looks good.

@karussell karussell merged commit 42f8074 into master Jun 28, 2019
@karussell karussell deleted the ext_pointlist branch June 28, 2019 14:12
@karussell
Copy link
Member Author

Ok. If there are problems we can easily create new issues :)

@karussell karussell changed the title Improve pointlist output for isochrone Improve pointlist output for isochrone (/spt endpoint) May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants