-
Notifications
You must be signed in to change notification settings - Fork 30
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
added e2e tests, some minor response parsing fixes #85
base: master
Are you sure you want to change the base?
Changes from 8 commits
58b708a
317e64f
3baad5c
0e10090
73bf55c
df30e1b
75ed8e4
f22759d
66ff7f0
da0c032
3e21ba5
25561fb
00fecc5
84ba3fe
8ccebbc
e7108c5
05e9876
514ad8e
764b20e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,3 +13,5 @@ test.py | |
*.pyc | ||
*.idea/ | ||
.DS_Store | ||
**/test_data/* | ||
!test_data/andorra-latest.osm.pbf |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -510,7 +510,7 @@ def isochrones( | |
type, | ||
intervals[0], | ||
buckets, | ||
center, | ||
locations, | ||
interval_type, | ||
) | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -519,7 +519,7 @@ def parse_isochrone_json(response, intervals, locations, interval_type): | |
Isochrone( | ||
geometry=feature["geometry"]["coordinates"], | ||
interval=intervals[idx], | ||
center=locations, | ||
center=locations[0], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is fine for now. but actually, the way we do it here is not really possible with valhalla. it's never returning more than one isochrone, even with 5 input locations. basically it does a geometric union of the isochrones from multiple locations, so we'll always have only one for valhalla we would need to do a bit more to support actual center coords. we return them conditionally with |
||
interval_type=interval_type, | ||
) | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
version: "3.8" | ||
services: | ||
ors: | ||
container_name: ors-tests | ||
ports: | ||
- "8080:8080" | ||
image: ghcr.io/gis-ops/openrouteservice:latest | ||
volumes: | ||
- ../test_data/andorra-latest.osm.pbf:/ors-core/data/osm_file.pbf | ||
environment: | ||
- "JAVA_OPTS=-Djava.awt.headless=true -server -XX:TargetSurvivorRatio=75 -XX:SurvivorRatio=64 -XX:MaxTenuringThreshold=3 -XX:+UseG1GC -XX:+ScavengeBeforeFullGC -XX:ParallelGCThreads=4 -Xms1g -Xmx2g" | ||
- "CATALINA_OPTS=-Dcom.sun.management.jmxremote -Dcom.sun.management.jmxremote.port=9001 -Dcom.sun.management.jmxremote.rmi.port=9001 -Dcom.sun.management.jmxremote.authenticate=false -Dcom.sun.management.jmxremote.ssl=false -Djava.rmi.server.hostname=localhost" | ||
valhalla: | ||
container_name: valhalla-tests | ||
image: gisops/valhalla:latest | ||
ports: | ||
- "8002:8002" | ||
volumes: | ||
- ../test_data/:/custom_files | ||
osrm: | ||
container_name: osrm-tests | ||
image: osrm/osrm-backend:latest | ||
ports: | ||
- "5000:5000" | ||
volumes: | ||
- ../test_data/:/data | ||
command: '/bin/bash -c "osrm-extract -p /opt/car.lua /data/andorra-latest.osm.pbf && osrm-partition /data/andorra-latest.osrm && osrm-customize /data/andorra-latest.osrm && osrm-routed --algorithm=MLD /data/andorra-latest.osrm"' | ||
graphhopper: | ||
container_name: graphhopper-tests | ||
image: israelhikingmap/graphhopper:latest | ||
ports: | ||
- "8989:8989" | ||
volumes: | ||
- ../test_data/:/graphhopper/data | ||
command: "-i data/andorra-latest.osm.pbf --host 0.0.0.0" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
import tests as _test | ||
from routingpy import ORS, OSRM, Graphhopper, Valhalla | ||
|
||
|
||
class TestRoutingpyIntegration(_test.TestCase): | ||
@classmethod | ||
def setUpClass(cls) -> None: | ||
cls.valhalla = Valhalla("http://localhost:8002") | ||
cls.osrm = OSRM("http://localhost:5000") | ||
cls.ors = ORS(base_url="http://localhost:8080/ors") | ||
cls.gh = Graphhopper(base_url="http://localhost:8989") | ||
|
||
def test_valhalla_directions(self): | ||
|
||
directions = self.valhalla.directions( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
], | ||
"auto", | ||
) | ||
|
||
self.assertIsInstance(directions.geometry, list) | ||
self.assertIsInstance(directions.duration, int) | ||
self.assertIsInstance(directions.distance, int) | ||
|
||
def test_valhalla_isochrones(self): | ||
|
||
isochrones = self.valhalla.isochrones( | ||
[ | ||
[1.51886, 42.5063], | ||
], | ||
"auto", | ||
[20, 50], | ||
) | ||
|
||
self.assertIsInstance(isochrones[1].geometry, list) | ||
self.assertEqual(isochrones[0].interval, 20) | ||
self.assertEqual(isochrones[1].interval, 50) | ||
self.assertEqual(isochrones[1].center, [1.51886, 42.5063]) | ||
|
||
def test_valhalla_matrix(self): | ||
matrix = self.valhalla.matrix( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
[1.53489, 42.52007], | ||
[1.53189, 42.51607], | ||
], | ||
"auto", | ||
) | ||
|
||
self.assertEqual(len(matrix.distances), 4) | ||
self.assertEqual(len(matrix.durations), 4) | ||
self.assertEqual(len(matrix.durations[0]), 4) | ||
self.assertEqual(len(matrix.distances[0]), 4) | ||
|
||
def test_osrm_directions(self): | ||
directions = self.osrm.directions( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
] | ||
) | ||
|
||
self.assertIsInstance(directions.geometry, list) | ||
self.assertIsInstance(directions.duration, int) | ||
self.assertIsInstance(directions.distance, int) | ||
|
||
def test_osrm_matrix(self): | ||
matrix = self.osrm.matrix( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
[1.53489, 42.52007], | ||
[1.53189, 42.51607], | ||
], | ||
"auto", | ||
) | ||
|
||
self.assertEqual(len(matrix.distances), 4) | ||
self.assertEqual(len(matrix.durations), 4) | ||
self.assertEqual(len(matrix.durations[0]), 4) | ||
self.assertEqual(len(matrix.distances[0]), 4) | ||
|
||
def test_ors_directions(self): | ||
directions = self.ors.directions( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
], | ||
"driving-car", | ||
) | ||
|
||
self.assertIsInstance(directions.geometry, list) | ||
self.assertIsInstance(directions.duration, int) | ||
self.assertIsInstance(directions.distance, int) | ||
|
||
def test_ors_isochrones(self): | ||
isochrones = self.ors.isochrones([1.51886, 42.5063], "driving-car", [20, 50]) | ||
|
||
self.assertIsInstance(isochrones[1].geometry, list) | ||
self.assertEqual(isochrones[0].interval, 20) | ||
self.assertEqual(isochrones[1].interval, 50) | ||
self.assertAlmostEqual(isochrones[1].center[0], 1.51886, 1) | ||
self.assertAlmostEqual(isochrones[1].center[1], 42.5063, 1) | ||
|
||
def test_ors_matrix(self): | ||
matrix = self.ors.matrix( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
[1.53489, 42.52007], | ||
[1.53189, 42.51607], | ||
], | ||
"driving-car", | ||
metrics=["distance", "duration"], | ||
) | ||
|
||
self.assertEqual(len(matrix.distances), 4) | ||
self.assertEqual(len(matrix.durations), 4) | ||
self.assertEqual(len(matrix.durations[0]), 4) | ||
self.assertEqual(len(matrix.distances[0]), 4) | ||
|
||
def test_graphhopper_directions(self): | ||
directions = self.gh.directions( | ||
[ | ||
[1.51886, 42.5063], | ||
[1.53789, 42.51007], | ||
], | ||
"car", | ||
) | ||
|
||
self.assertIsInstance(directions.geometry, list) | ||
self.assertIsInstance(directions.duration, int) | ||
self.assertIsInstance(directions.distance, int) | ||
|
||
def test_graphhopper_isochrones(self): | ||
isochrones = self.gh.isochrones([1.51886, 42.5063], "car", [50]) | ||
|
||
self.assertIsInstance(isochrones[0].geometry, list) | ||
self.assertEqual(isochrones[0].interval, 50) | ||
self.assertAlmostEqual(isochrones[0].center[0], 1.51886, 1) | ||
self.assertAlmostEqual(isochrones[0].center[1], 42.5063, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I understand this (and was wrong before too, but don't think it's actually solved now):
locations
is an array right? If you look atparse_isochrone_json
, it'll be put ascenter
for an individual isochrone. So down there it's missing an indexing into thecenter
array.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a couple of things here: the parameter is
locations
when really, it's expecting one location as[lon, lat]
so it already represents a single location. On top of that,center
is really just that location but the coordinates are represented as strings:https://github.com/gis-ops/routingpy/blob/73bf55cdcb8871c1c6b9886bc14a5ca8aef9a38f/routingpy/routers/graphhopper.py#L490
The real problem seems to be the confusion about the name of the
locations
parameter: Valhalla, ORS, GraphHopper and HERE all only accept one single location, yet that parameter is consistently named in the plural form. We could either consistently change them tolocation
or we accept multiple and fire multiple requests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What adds even more to the confusion is that Valhalla takes one location but nested into an array, but GraphHopper and ORS take one location (unnested).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost: Valhalla & ORS do accept multiple locations, but only ORS is doing what the code assumes, Valhalla does some weird union stuff with the multiple locations. Since we just came from ORS when we started routingpy, we tried to support that. In the end, I agree, let's just get rid of supporting multiple locations and accept only a single location for isochrones. Major version sort of thing, but no problem to push one, I don't really care tbh.