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

Isochrone Plugin #2189 #2477

Open
wants to merge 63 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@roroettg

roroettg commented May 27, 2016

I hope I am doing this right.

As requested in #2189 this pullrequest contains the isochrone plugin I am working on.

  • The plugin is optional and can be set with parameter -I or --isochrone
  • No additional data gets loaded then the plugin is not used

roroettg added some commits Mar 17, 2016

Merge branch 'develop' of https://github.com/beemogmbh/osrm-backend i…
…nto develop

Conflicts:
	features/bicycle/range.feature
	include/engine/api/isochrone_api.hpp
	include/engine/api/isochrone_parameters.hpp
	include/engine/plugins/isochrone.hpp
	src/engine/engine.cpp
	src/engine/plugins/isochrone.cpp
Merge remote-tracking branch 'upstream/master'
Conflicts:
	src/storage/storage_config.cpp
@MoKob

This comment has been minimized.

Show comment
Hide comment
@MoKob

MoKob Jul 20, 2016

@roroettg well. Given that OSRM uses a CH, you have to introduce a parallel graph for your plugin to work. I assume that you are using Dijkstras algorithm on the plane Edge-Based-Graph?

I have to say that I am probably not the best person to decide how this is going to work. But I feel that a plugin that requires a full parallel graph might be hard to integrate fully into OSRM. /cc @TheMarex

MoKob commented Jul 20, 2016

@roroettg well. Given that OSRM uses a CH, you have to introduce a parallel graph for your plugin to work. I assume that you are using Dijkstras algorithm on the plane Edge-Based-Graph?

I have to say that I am probably not the best person to decide how this is going to work. But I feel that a plugin that requires a full parallel graph might be hard to integrate fully into OSRM. /cc @TheMarex

@TheMarex

This comment has been minimized.

Show comment
Hide comment
@TheMarex

TheMarex Jul 20, 2016

Member

@roroettg that looks pretty cool. Thanks for this contribution. I would like to get this properly reviewed and then think about if (and how) we can move this forward to be included in upstream. Biggest blocker here is currently time constraints on our side.

The biggest problem that we face is how we could handle optional plugins. Currently our library interface exposes a function for each service:

class OSRM final
{
  public:
    explicit OSRM(EngineConfig &config);

    ~OSRM();

    // Moveable but not copyable
    OSRM(OSRM &&) noexcept;
    OSRM &operator=(OSRM &&) noexcept;

    Status Route(const RouteParameters &parameters, json::Object &result);
    Status Table(const TableParameters &parameters, json::Object &result);
    Status Nearest(const NearestParameters &parameters, json::Object &result);
    Status Trip(const TripParameters &parameters, json::Object &result);
    Status Match(const MatchParameters &parameters, json::Object &result);
    Status Tile(const TileParameters &parameters, std::string &result);

  private:
    std::unique_ptr<engine::Engine> engine_;
};

If we now disable a service that would mean we have a function that would do nothing? @daniel-j-h do you have an idea how we could solve this in a C++-y way?

Conditionally loading additional data is something that we should figure out down the road anyway (why would you load lane guidance if you only want to run distance table requests etc..), so I'm not too concerned here.

The command line flag is a first good step, but we might want a more general interface here.

Just skimmed the diff, did you run script/format.sh? It only works correctly with clang-format 3.8 I think so you would probably need to return this.

Member

TheMarex commented Jul 20, 2016

@roroettg that looks pretty cool. Thanks for this contribution. I would like to get this properly reviewed and then think about if (and how) we can move this forward to be included in upstream. Biggest blocker here is currently time constraints on our side.

The biggest problem that we face is how we could handle optional plugins. Currently our library interface exposes a function for each service:

class OSRM final
{
  public:
    explicit OSRM(EngineConfig &config);

    ~OSRM();

    // Moveable but not copyable
    OSRM(OSRM &&) noexcept;
    OSRM &operator=(OSRM &&) noexcept;

    Status Route(const RouteParameters &parameters, json::Object &result);
    Status Table(const TableParameters &parameters, json::Object &result);
    Status Nearest(const NearestParameters &parameters, json::Object &result);
    Status Trip(const TripParameters &parameters, json::Object &result);
    Status Match(const MatchParameters &parameters, json::Object &result);
    Status Tile(const TileParameters &parameters, std::string &result);

  private:
    std::unique_ptr<engine::Engine> engine_;
};

If we now disable a service that would mean we have a function that would do nothing? @daniel-j-h do you have an idea how we could solve this in a C++-y way?

Conditionally loading additional data is something that we should figure out down the road anyway (why would you load lane guidance if you only want to run distance table requests etc..), so I'm not too concerned here.

The command line flag is a first good step, but we might want a more general interface here.

Just skimmed the diff, did you run script/format.sh? It only works correctly with clang-format 3.8 I think so you would probably need to return this.

@agponte agponte referenced this pull request Aug 29, 2016

Closed

full usage instructions #14

roroettg added some commits Sep 1, 2016

roroettg added some commits Sep 8, 2016

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Dec 4, 2016

Member

I have a possible strategy that does not require loading the edge-based-graph into memory at all - while we can't explore the CH directly, we can re-create the parts of the edge-based-graph we need on-the-fly.

The recent work I did on the debug tile plugin to add turn penalty data involved the re-derivation of the edge-based graph within the tile being queried. This is possible by fetching all data in a geographic area, then re-connecting the dots. See this code.

The rough idea would be to come up with a reasonable "search tile" size (my gut says 5km x 5km would be good starting point). The rough algorithm would be:

  1. Fetch a "tile" of data around our isochrone start point.
  2. Construct the local edge-based-graph from that data
  3. Begin our isochrone search
  4. If we hit the edge of our tile without terminating, fetch the neighbor tile and extend our edge-based-mini-graph
  5. Continue the isochrone search, repeating (4) as necessary until the search ends.

Performance would be dictated by (a) the cost of the bbox RTree search to fetch tile data, and (b) the cost of re-constructing the local edge-based-graph from that tile. For the example ranges above by @roroettg , I suspect we could fetch all data in a single "tile".

Member

danpat commented Dec 4, 2016

I have a possible strategy that does not require loading the edge-based-graph into memory at all - while we can't explore the CH directly, we can re-create the parts of the edge-based-graph we need on-the-fly.

The recent work I did on the debug tile plugin to add turn penalty data involved the re-derivation of the edge-based graph within the tile being queried. This is possible by fetching all data in a geographic area, then re-connecting the dots. See this code.

The rough idea would be to come up with a reasonable "search tile" size (my gut says 5km x 5km would be good starting point). The rough algorithm would be:

  1. Fetch a "tile" of data around our isochrone start point.
  2. Construct the local edge-based-graph from that data
  3. Begin our isochrone search
  4. If we hit the edge of our tile without terminating, fetch the neighbor tile and extend our edge-based-mini-graph
  5. Continue the isochrone search, repeating (4) as necessary until the search ends.

Performance would be dictated by (a) the cost of the bbox RTree search to fetch tile data, and (b) the cost of re-constructing the local edge-based-graph from that tile. For the example ranges above by @roroettg , I suspect we could fetch all data in a single "tile".

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Dec 4, 2016

Member

Ok, I just had a go at this to see what kind of performance we'd get. It was relatively easy to copy the logic from the tile plugin to create the edge-based-graph we need.

100 second radius
screen shot 2016-12-04 at 1 32 55 am
200 second radius
screen shot 2016-12-04 at 1 32 35 am
300 second radius
screen shot 2016-12-04 at 1 32 42 am

A 400-second isochrone on the car profile is rendered in about 200ms, including the generation of 1.2MB of GeoJSON. Its not doing the tile loading, but it's loading a 0.2 degree x 0.2 degree box (about 10km x 10km in Münster).

@roroettg For your use-case, how large were you planning on allowing the isochrones? If I push my code up, do you think you could adapt your concave hull to it?

Member

danpat commented Dec 4, 2016

Ok, I just had a go at this to see what kind of performance we'd get. It was relatively easy to copy the logic from the tile plugin to create the edge-based-graph we need.

100 second radius
screen shot 2016-12-04 at 1 32 55 am
200 second radius
screen shot 2016-12-04 at 1 32 35 am
300 second radius
screen shot 2016-12-04 at 1 32 42 am

A 400-second isochrone on the car profile is rendered in about 200ms, including the generation of 1.2MB of GeoJSON. Its not doing the tile loading, but it's loading a 0.2 degree x 0.2 degree box (about 10km x 10km in Münster).

@roroettg For your use-case, how large were you planning on allowing the isochrones? If I push my code up, do you think you could adapt your concave hull to it?

@Komzpa

This comment has been minimized.

Show comment
Hide comment
@Komzpa

Komzpa Dec 4, 2016

Contributor

@danpat just in case, we need 30 minute car isochrones with RPS of 50k. It looks like to achieve that some caching is needed.

Contributor

Komzpa commented Dec 4, 2016

@danpat just in case, we need 30 minute car isochrones with RPS of 50k. It looks like to achieve that some caching is needed.

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Dec 4, 2016

Member

@Komzpa RPS == Requests Per Second, or https://xkcd.com/645/ ?

Member

danpat commented Dec 4, 2016

@Komzpa RPS == Requests Per Second, or https://xkcd.com/645/ ?

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Dec 4, 2016

Member

screen shot 2016-12-04 at 11 47 30 am

@Komzpa ^^ this is a 30 minute driving isochrone centered on the same point as the above examples.

On my laptop (with an i7 CPU), it takes about 6.3 seconds, broken down into:

  • Fetch a 2x2 degree bounding box from the RTree - 102.185ms
  • Create an initial lookup table of edges - 283ms
  • Constrct the edge-based-graph - 383ms
  • Perform the dijkstra search on the edge-based-graph - 4869ms
  • Geojson serialization - ~900ms

This approach is basically the worst case for isochrone generation - it employs no speedups at all.

I'm of the opinion that even a limited isochrone feature would be good to have - the demo server can easily restrict the allowed isochrone size to something that won't eat up all our CPU, and folks that want to do bigger ones can set up their own servers.

Additional memory use is 🙅 for the demo server, but I think limited isochrone sizes would be just fine.

Member

danpat commented Dec 4, 2016

screen shot 2016-12-04 at 11 47 30 am

@Komzpa ^^ this is a 30 minute driving isochrone centered on the same point as the above examples.

On my laptop (with an i7 CPU), it takes about 6.3 seconds, broken down into:

  • Fetch a 2x2 degree bounding box from the RTree - 102.185ms
  • Create an initial lookup table of edges - 283ms
  • Constrct the edge-based-graph - 383ms
  • Perform the dijkstra search on the edge-based-graph - 4869ms
  • Geojson serialization - ~900ms

This approach is basically the worst case for isochrone generation - it employs no speedups at all.

I'm of the opinion that even a limited isochrone feature would be good to have - the demo server can easily restrict the allowed isochrone size to something that won't eat up all our CPU, and folks that want to do bigger ones can set up their own servers.

Additional memory use is 🙅 for the demo server, but I think limited isochrone sizes would be just fine.

@roroettg

This comment has been minimized.

Show comment
Hide comment
@roroettg

roroettg Dec 5, 2016

@danpat Nice Work. For our use-case isochrones with the size of up to two hours for a bicycle profile would be perfect. With my current approach the memory is most of the time the limit when the isochrones get too big. On a different branch I separated the isochrone-service from the current routed-service to avoid having two graphs at runtime.

@danpat Yes, probably i can adapt my code.

roroettg commented Dec 5, 2016

@danpat Nice Work. For our use-case isochrones with the size of up to two hours for a bicycle profile would be perfect. With my current approach the memory is most of the time the limit when the isochrones get too big. On a different branch I separated the isochrone-service from the current routed-service to avoid having two graphs at runtime.

@danpat Yes, probably i can adapt my code.

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Dec 5, 2016

Member

Yeah, the memory problem....I think we just need to make the limit configurable at runtime - if the average travel speed is slow, then 2 hrs may not actually be all that big of a maximum radius.

I'll push my changes up to a separate branch here and you can take a look.

Member

danpat commented Dec 5, 2016

Yeah, the memory problem....I think we just need to make the limit configurable at runtime - if the average travel speed is slow, then 2 hrs may not actually be all that big of a maximum radius.

I'll push my changes up to a separate branch here and you can take a look.

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Dec 5, 2016

Member

Pushed to here: https://github.com/Project-OSRM/osrm-backend/tree/ch_isochrone_poc

This doesn't implement the tiled approach, it simply fetches a bounding box of data using a maximum-possible-radius calculation (200km/h for cars). The nice thing about this is that it scales down quite well - small isochrones are quite fast.

My code is simply returning the geometry of all edges inside the isochrone boundary - there's no data on points for age, and the data is not tree-like in anyway.

That said, these geometry adaptations should be relatively straight forward. It could also probably get quite a bit faster with some work on cache locality.

I spent a bit of time over the weekend reading up on one-to-many speedup approaches - I think there are some options for future work. For now, I'd just like to get a workable plugin in, and get folks using it, then we can get feedback on what needs to work better.

One thing I'd be very interested in knowing: do folks care for exact isochrone trees, or are approximate bounding polygons more useful?

Member

danpat commented Dec 5, 2016

Pushed to here: https://github.com/Project-OSRM/osrm-backend/tree/ch_isochrone_poc

This doesn't implement the tiled approach, it simply fetches a bounding box of data using a maximum-possible-radius calculation (200km/h for cars). The nice thing about this is that it scales down quite well - small isochrones are quite fast.

My code is simply returning the geometry of all edges inside the isochrone boundary - there's no data on points for age, and the data is not tree-like in anyway.

That said, these geometry adaptations should be relatively straight forward. It could also probably get quite a bit faster with some work on cache locality.

I spent a bit of time over the weekend reading up on one-to-many speedup approaches - I think there are some options for future work. For now, I'd just like to get a workable plugin in, and get folks using it, then we can get feedback on what needs to work better.

One thing I'd be very interested in knowing: do folks care for exact isochrone trees, or are approximate bounding polygons more useful?

@Komzpa

This comment has been minimized.

Show comment
Hide comment
@Komzpa

Komzpa Dec 6, 2016

Contributor

@danpat
Thanks for the benchmark! :)
We do care for exact isochrone trees. At very least, there are different travel times to left and right sides of the two-way streets, up to serveral minutes - we need to distinguish those.

Contributor

Komzpa commented Dec 6, 2016

@danpat
Thanks for the benchmark! :)
We do care for exact isochrone trees. At very least, there are different travel times to left and right sides of the two-way streets, up to serveral minutes - we need to distinguish those.

@DrewRay

This comment has been minimized.

Show comment
Hide comment
@DrewRay

DrewRay Jan 19, 2017

Happy New year, is this still blocked by merge issues? is it even a priority to keep working on?

DrewRay commented Jan 19, 2017

Happy New year, is this still blocked by merge issues? is it even a priority to keep working on?

@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Jan 19, 2017

Member

@DrewRay as far as I know this isn't a priority for anyone, thus the lack of progress.

If you've got time and are willing to work on it, please feel free :-) For my branch a couple of comments back, the thing I got stalled on was making a reasonably compact JSON-like serialization of the isochrone tree data. GeoJSON doesn't really work for this.

Member

danpat commented Jan 19, 2017

@DrewRay as far as I know this isn't a priority for anyone, thus the lack of progress.

If you've got time and are willing to work on it, please feel free :-) For my branch a couple of comments back, the thing I got stalled on was making a reasonably compact JSON-like serialization of the isochrone tree data. GeoJSON doesn't really work for this.

@frodrigo

This comment has been minimized.

Show comment
Hide comment
@frodrigo

frodrigo Jan 23, 2017

Member

One thing I'd be very interested in knowing: do folks care for exact isochrone trees, or are approximate bounding polygons more useful?

On my side, each time I need isochrones it is for the hull, never for the tree.

Member

frodrigo commented Jan 23, 2017

One thing I'd be very interested in knowing: do folks care for exact isochrone trees, or are approximate bounding polygons more useful?

On my side, each time I need isochrones it is for the hull, never for the tree.

@frodrigo

This comment has been minimized.

Show comment
Hide comment
@frodrigo

frodrigo Jan 23, 2017

Member

It would be nice to have the maxspeed as parameter.

Member

frodrigo commented Jan 23, 2017

It would be nice to have the maxspeed as parameter.

@DrewRay

This comment has been minimized.

Show comment
Hide comment
@DrewRay

DrewRay Jan 23, 2017

DrewRay commented Jan 23, 2017

@frodrigo

This comment has been minimized.

Show comment
Hide comment
@frodrigo

frodrigo Jan 24, 2017

Member

I test this branch and got two issues:

Member

frodrigo commented Jan 24, 2017

I test this branch and got two issues:

public:
explicit IsochronePlugin(const std::string base);
Status HandleRequest(const std::shared_ptr<datafacade::BaseDataFacade> facade,

This comment has been minimized.

@fijemax

fijemax Feb 3, 2017

Contributor

This can't compile datafacade::BaseDataFacade need to be const.

@fijemax

fijemax Feb 3, 2017

Contributor

This can't compile datafacade::BaseDataFacade need to be const.

@fijemax

This comment has been minimized.

Show comment
Hide comment
@fijemax

fijemax Feb 3, 2017

Contributor

The ch_isochrone_poc branch doesn't work since last rebased on master branch.
The query blocks the osrm-routed, it can't stop correctly :

./build/osrm-routed ../pbf-data/map.osrm
[info] starting up engines, v5.5.0
[info] Threads: 8
[info] IP address: 0.0.0.0
[info] IP port: 5000
[info] http 1.1 compression handled by zlib version 1.2.8
[info] Listening on: 0.0.0.0:5000
[info] load names from: "../pbf-data/map.osrm.names"
[info] set checksum: 4022734474
[info] running and waiting for requests
^C[info] initiating shutdown
[info] stopping threads
[warn] Didn't exit within 2 seconds. Hard abort!
terminate called after throwing an instance of 'std::future_error'
what(): std::future_error: No associated state
Abandon (core dumped)

Contributor

fijemax commented Feb 3, 2017

The ch_isochrone_poc branch doesn't work since last rebased on master branch.
The query blocks the osrm-routed, it can't stop correctly :

./build/osrm-routed ../pbf-data/map.osrm
[info] starting up engines, v5.5.0
[info] Threads: 8
[info] IP address: 0.0.0.0
[info] IP port: 5000
[info] http 1.1 compression handled by zlib version 1.2.8
[info] Listening on: 0.0.0.0:5000
[info] load names from: "../pbf-data/map.osrm.names"
[info] set checksum: 4022734474
[info] running and waiting for requests
^C[info] initiating shutdown
[info] stopping threads
[warn] Didn't exit within 2 seconds. Hard abort!
terminate called after throwing an instance of 'std::future_error'
what(): std::future_error: No associated state
Abandon (core dumped)

@danpat danpat referenced this pull request Feb 4, 2017

Open

Isochrone Plugin v2 #3652

0 of 7 tasks complete
@danpat

This comment has been minimized.

Show comment
Hide comment
@danpat

danpat Feb 4, 2017

Member

@fijemax I've rebased and removed some WIP code from that branch, and opened a new PR at #3652. It's in a working state right now.

Member

danpat commented Feb 4, 2017

@fijemax I've rebased and removed some WIP code from that branch, and opened a new PR at #3652. It's in a working state right now.

@mloskot

This comment has been minimized.

Show comment
Hide comment
@mloskot

mloskot Aug 7, 2017

Contributor

Is this proposal deprecated (to-be-closed) as superseded by #3652 or both are being developed as alternative approaches?

Contributor

mloskot commented Aug 7, 2017

Is this proposal deprecated (to-be-closed) as superseded by #3652 or both are being developed as alternative approaches?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment