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 #1577

Merged
merged 21 commits into from Jun 28, 2019

Conversation

@karussell
Copy link
Member

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 added 2 commits Mar 25, 2019
@crazycapivara

This comment has been minimized.

Copy link

crazycapivara commented on d29c7ca Mar 26, 2019

It returns distance for time as well:

switch (h) {
                        case "distance":
                            list.add(label.distance);
                            break;
                        case "time":
                            list.add(label.distance);
                            break;
@karussell karussell changed the title Improve pointlist for isochrone Improve pointlist output for isochrone Mar 26, 2019
@crazycapivara

This comment has been minimized.

Copy link

crazycapivara commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Member Author

karussell commented Mar 28, 2019

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

This comment has been minimized.

Copy link
Member Author

karussell commented Mar 28, 2019

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

lat <- 52.509535
lng <- 13.40332
@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Mar 28, 2019

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

This comment has been minimized.

Copy link

crazycapivara commented Mar 28, 2019

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

@crazycapivara

This comment has been minimized.

Copy link

crazycapivara commented Mar 28, 2019

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

This comment has been minimized.

Copy link

crazycapivara commented Mar 28, 2019

graphhopper-deckgl-R

@crazycapivara

This comment has been minimized.

Copy link

crazycapivara commented Apr 1, 2019

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

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 1, 2019

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

This comment has been minimized.

Copy link

crazycapivara commented Apr 2, 2019

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

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 2, 2019

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

This comment has been minimized.

Copy link

crazycapivara commented Apr 3, 2019

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

This comment has been minimized.

Copy link

crazycapivara commented Apr 3, 2019

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 5, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 10, 2019

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

@michaz

This comment has been minimized.

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

This comment has been minimized.

Copy link

crazycapivara commented Apr 12, 2019

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

@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 12, 2019

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

This comment has been minimized.

Copy link
@Komzpa

Komzpa Apr 18, 2019

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;

This comment has been minimized.

Copy link
@karussell

karussell Apr 26, 2019

Author Member

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

This comment has been minimized.

Copy link
@Komzpa

Komzpa Apr 26, 2019

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

karussell commented Apr 26, 2019

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

This comment has been minimized.

Copy link
Member Author

karussell commented May 31, 2019

Great - thanks for the feedback! Will then move this endpoint to CSV-only (which would have the advantage of streeming in later implementations)

@karussell

This comment has been minimized.

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 added 2 commits May 31, 2019
@karussell

This comment has been minimized.

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 karussell referenced this pull request May 31, 2019
7 of 7 tasks complete
@karussell

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link

crazycapivara commented Jun 4, 2019

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

karussell commented Jun 8, 2019

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

This comment has been minimized.

Copy link

crazycapivara commented Jun 8, 2019

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

karussell added 2 commits Jun 14, 2019
@karussell

This comment has been minimized.

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 added 2 commits Jun 15, 2019
@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Jun 17, 2019

@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

This comment has been minimized.

Copy link
Member Author

karussell commented Jun 28, 2019

If no opposing opinion I'll merge this today

@Komzpa

This comment has been minimized.

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
5 checks passed
5 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
security/snyk - pom.xml (karussell) No new issues
Details
security/snyk - web/package.json (karussell) No manifest changes detected
@karussell karussell deleted the ext_pointlist branch Jun 28, 2019
@karussell

This comment has been minimized.

Copy link
Member Author

karussell commented Jun 28, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.