Add Average Speed as PathDetails to Response #1091

boldtrn · 2017-06-17T13:23:00Z

Fixes #439.

I added the possibility to view details of the calculated as proposed in #439. It should be very easy to extend so we can add different details.

The calculation of the PathDetails is a bit similar to Instructions and maybe in the future we might find that it makes sense to merge them, but not now.

I haven't measured the performance yet, but I can do this if you like the general approach.

Simplification is disabled when adding PathDetails right now.

Right now I don't like two things:

The data structure, not an issue right now, but maybe if we output osm ids for continental requests? Also see the conversion in the SimpleRouteSerializer. I think, wrapping the PathDetails in a Map again, might be more efficient. Overall it is a bit too much map usage IMHO, but it seems like the right data structure ;).
The initialization happens in the GraphHopper class, right now not an issue, but once we have more details, we might want to move this somewhere else.

Future improvements:
Change the way the Instructions are generated so that we generate the intervals for the instructions in the PathMerger. Then we could try to simplify points between PathDetails/Instructions without simplifying over the bounds of any of the two.

devemux86 · 2017-06-17T13:56:33Z

core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java

+        lat = nodeAccess.getLat(baseNode);
+        lng = nodeAccess.getLon(baseNode);
+        for (; pointIndex < points.size(); pointIndex++) {
+            if (lat == points.getLat(pointIndex) && lng == points.getLon(pointIndex)) {


Is it safe comparing coordinates without epsilon?

It should be, since they come from the same source? I could add a delta, though. But it's good that you remind me, I wanted to check how the algorithm behaves for loops etc.

karussell · 2017-06-17T14:30:33Z

Thanks!

Check how the algorithm behaves for loops, dead ends, etc.

Maybe just run 100K routes for Germany via the Measurement class :) ?

karussell · 2017-06-17T14:32:09Z

Also see the conversion in the SimpleRouteSerializer

It might be interesting here or in a separate issue to get rid of this class entirely as we now could let jackson do this serialization

boldtrn · 2017-06-17T14:37:28Z

Maybe just run 100K routes for Germany via the Measurement class :) ?

Ah, I meant if the correct nodes are matched, just because at a crossing of the route it could be point=50 on the first way but point 270 on the way back, with waypoints we start the counting from the beginning for every path, but after every waypoint a new path starts, but we need to consider the points from the start to the end of the path to calculate the correct index, so maybe we have to remember the last index or something like that (not sure if this is understandable, not easy to explain without code)

For example a route like this: https://graphhopper.com/maps/?point=49.368066%2C9.703674&point=49.396452%2C9.753628&point=49.375332%2C9.75071&point=49.38606%2C9.683933

When comparing only coordinates we might produce incorrect results when the routes cross.

It might be interesting here or in a separate issue to get rid of this class entirely as we now could let jackson do this serialization

👍

karussell · 2017-06-17T14:43:57Z

Sorry didn't yet look into the source code. Why do we need coordinate comparison?

boldtrn · 2017-06-17T14:46:59Z

Sorry didn't yet look into the source code. Why do we need coordinate comparison?

Not sure if we need it, I used this to calculate the interval, but now that you mention it, we could probably also count the number of points per edge and just sum it up.

karussell · 2017-06-17T14:48:25Z

Yes, please :). To make this then more secure we need probably the same tests that you mention but matching is only good if we have no other choice

boldtrn · 2017-06-27T15:43:28Z

I updated the code, it's a lot better now, compared to coordinate comparing. I added a rather ugly js debugging code to check if the intervals are set correctly, see the attached screenshot:

boldtrn · 2017-07-01T12:49:33Z

I updated this PR so that the PathDetails are now stored similar to the instructions, so every PathDetail knows the number of points it contains. The benefit here is that when we run the simplification we don't have to update every PathDetail, but only the ones that are actually changed.

When serializing I run a similar approach as the instruction to create actual intervals.

I saw a "minor issue" close to waypoints. Sometime the start of the PathDetails move slightly when the waypoints are placed close to the beginning of an edge, where the PathDetails would change. Not sure what's the reason, but I think it could have something to do with VirtualEdges, but haven't checked, since I am not sure if it's worth the trouble, see the attached screenshots.

The route is in Baden-Württemberg: http://localhost:8989/?point=48.596365%2C8.634396&point=48.586398%2C8.618098&point=48.578155%2C8.609955&details.average_speed=true&debug_details=true

This is the OSM way.

karussell · 2017-07-03T13:00:16Z

Thanks! Will have a look

I saw a "minor issue" close to waypoints

I think it is important to understand the issue to judge if we should fix it or not.

karussell · 2017-07-03T13:19:35Z

web/src/main/java/com/graphhopper/http/SimpleRouteSerializer.java

+                        for (PathDetails pathDetails : details) {
+                            pathDetailsMap.put(pathDetails.getName(), pathDetails.getPathDetailsMap());
+                        }
+                        jsonPath.put("details", pathDetailsMap);


Can't we use a Map when returning from ar.getPathDetails so that we could write jsonPath.put("details", ar.getPathDetails())? Or annotate PathDetails so that we get the JSON output that we want via jackson.

Yes, makes sense 👍

karussell · 2017-07-03T13:22:59Z

core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java

+public class PathDetailsFromEdges implements Path.EdgeVisitor{
+
+    int i;
+    PathDetailsCalculator calc;


Would you mind to move everything inside the loop? This reads confusing.

karussell · 2017-07-03T13:24:48Z

core/src/main/java/com/graphhopper/util/details/PathDetailsFromEdges.java

+            calc = calculators.get(i);
+            if (calc.edgeIsDifferentToLastEdge(edge)) {
+                details.get(i).endInterval(numberOfPoints);
+                details.get(i).startInterval(calc.getCurrentValue());


It reads a bit strange that the index is used for calculators and details. Can we somehow assign one calculator to the PathDetails?

Also can we avoid stateful stuff like with the .reset call? (if possible at all)

Don't you think that it makes sense to have calculator per detail? Essentially every calculator handles one PathDetail. So if you want to add a PathDetail in the Response, you have to pass the calculator to the PathMerger.

But why should we use two independent lists? Why not e.g. setting the calculator as variable of PathDetails?

Also can we avoid stateful stuff like with the .reset call? (if possible at all)

I don't like the reset call either. Mhm, we could create a new calculator for every Path. However, I think it makes sense in this case that the Calculator has a state and that we don't have to keep the state outside of the calculator. This allows us to handle complex states, using multiple variables in the calculator, but we don't have to care about it from the outside. WDYT?

Why not create fresh calculators for every Path?

Why not create fresh calculators for every Path?

On the other hand, why creating the overhead :)?

I think we could create a better setup here. In the GraphHopper.java class we could create a PathDetailsFactory that is initialized with the GHRequest. It reads the request and can create fresh calculators on request?

karussell · 2017-07-03T13:26:51Z

core/src/main/java/com/graphhopper/util/details/PathDetails.java

+        return pathDetails;
+    }
+
+    public void merge(PathDetails pD) {


Why do we need the merge method?

To merge the PathDetails for every Path. We could also do this completely in the PathMerger class. At first I created it because PathDetails were a map and it made a lot of sense then. With the current code it could be removed and done in the PathMerger.

karussell · 2017-07-03T13:27:57Z

core/src/main/java/com/graphhopper/util/details/AverageSpeedDetails.java

+
+    @Override
+    public String getName() {
+        return "average-speed";


Potentially we should use the same name average_speed like we use for the parameter in the URL?

karussell · 2017-07-03T13:28:59Z

core/src/main/java/com/graphhopper/util/PathMerger.java

@@ -86,8 +95,8 @@ public void doWork(PathWrapper altRsp, List<Path> paths, Translation tr) {
                    }

                    for (Instruction i : il) {
+                        origPoints += i.getPoints().size();


Was this a bug in the previous state of code?

Yes. Well it was only used for logging, so it was not really bad, also it only appeared when having multiple Paths. I thought about creating an issue for it but then I thought it's not worth to create an issue for this.

karussell · 2017-07-03T13:30:12Z

core/src/main/java/com/graphhopper/routing/Path.java

+        List<PathDetails> details = new ArrayList<>(calculators.size());
+        for (PathDetailsCalculator calc : calculators) {
+            details.add(new PathDetails(calc.getName()));
+            calc.reset();


Why do we have to call reset for the initialization?

karussell · 2017-07-03T13:31:02Z

core/src/main/java/com/graphhopper/PathWrapper.java

+                this.pathDetails.get(i).merge(details.get(i));
+            }
+        }
+    }


See below: what is a 'merge'? And why not just adding PathDetails like the method says it?

Also instead of EMPTY_LIST and if+else I would prefer without the if and always do the same.

boldtrn · 2017-07-04T17:16:36Z

I think it is important to understand the issue to judge if we should fix it or not.

Ok, so I tried to dig into this issue a bit. I compared these two routes:
http://localhost:8989/?point=48.58802%2C8.618581&point=48.586491%2C8.617648&point=48.580355%2C8.606071&details.average_speed=true&debug_details=true
http://localhost:8989/?point=48.58802%2C8.618581&point=48.586469%2C8.61798&point=48.580355%2C8.606071&details.average_speed=true&debug_details=true

I understood the issue now. There is one point in the resulting points that is reserved for the via point that cannot be calculated via sumarizing points of edges but has to be plainly added if there is a via point.

boldtrn · 2017-07-11T09:36:23Z

I updated this PR. I added a way to simplify the Path for PathDetails+Instructions. So the simplification keeps the points in between the different Instructions and PathDetails.

Optically, when using the Debugging view, the results look correct. I can also see that there is a difference of the response size if I pass way_point_max_distance=0 in the URL (getting bigger, as no actual simplification is triggered). The location of the average speed as well as instructions nodes look correct.

I would have expected that the GHResponse contains the same amount of points as before if it only contains instructions. This is not true right now. We have less points, I am not exactly sure if this indicates an issue or not. I haven't found the reason yet.

This is also the reason why the tests are failing right now.

karussell · 2017-08-11T10:34:20Z

The merge method should be easier as we can just call pathDetails.setLast(otherDetail.getLast()) but I didn't get the simplification working easily. Would you have a look?

karussell · 2017-08-11T10:37:03Z

core/src/main/java/com/graphhopper/util/PathSimplification.java

                    int removed = douglasPeucker.simplify(pointList, nonConflictingStart, nonConflictingEnd);
                    if (removed > 0) {
                        for (int i = 0; i < toSimplify.size(); i++) {
-                            reduceNumberOfPoints(toSimplify.get(i), offset[i], removed, startIntervals[i], endIntervals[i] - removed);


Here I didn't understood what nonConflictingXY means ... I think we have to call this for all PathDetails even if no simplification is possible, as we need to change the pathDetail.first variable for all entries following a simplified PathDetail.

The nonConflictingXY describes the index from where to where we can simplify without potentially remove a point that we might have to reference later.

I think we have to call this for all PathDetails even if no simplification is possible, as we need to change the pathDetail.first variable for all entries following a simplified PathDetail.

Yes this is true, since we don't store the length anymore, but the indexes we have to propagate our change trough all PathDetails, which is not very nice. That's btw the main reason why I decided earlier to only use length instead of indexes.

Yes this is true, since we don't store the length anymore, but the indexes we have to propagate our change trough all PathDetails, which is not very nice. That's btw the main reason why I decided earlier to only use length instead of indexes.

Ok, I just checked, we actually don't necessarily have to do this. Right now the Simplification is completely based on the length and does not use the index of the toSimplify elements at all, since the length of elements is fixed until they are actually simplified, it still works.

If you check how the startInterval and endInterval are managed, you will see that we don't use the index, but only the length of the elements. However, it looks a bit like "magic" and you don't understand it on first sight, which I generally don't like. On the other hand the simplification is a bit more complex anyway so it might be ok, that the code is not completely understandable by just skimming over it.

boldtrn · 2017-08-14T05:07:18Z

core/src/main/java/com/graphhopper/util/PathSimplification.java

-                endReached = true;
-            }
+            if (offset[toShiftIndex] >= toSimplify.get(toShiftIndex).size())
+                break;


should we just return pointList?

If possible I try to return at the end of a method

Ok, I am fine with that :)

boldtrn · 2017-08-14T07:01:29Z

core/src/main/java/com/graphhopper/util/PathSimplification.java

@@ -98,6 +98,16 @@ public PointList simplify() {
                    if (removed > 0) {
                        for (int i = 0; i < toSimplify.size(); i++) {
                            reduceLength(toSimplify.get(i), offset[i], startIntervals[i], endIntervals[i] - removed);
+                            /*


As you can see, the tests succeed even if we don't do this. Also the result on the map with debug=true looks the same to me in both cases, so this seems unnecessary. As described in my other comment and I would remove it before we merge this. I just left it there so you can compare it.

Ah, I think I have now fully understood the complexity, so when there are n PathDetails we do n simplifications and roughly n²/2 updates to the PathDetail object. The only alternative would be to sort the PathDetails by first index, create a dependency graph and then being to only update the PathDetails that are necessary

But this is not necessary as a first version (and probably even later)

Ah, I think I have now fully understood the complexity, so when there are n PathDetails we do n simplifications

No, we only have one PointList that we simplify. We only update the PahtDetails and Instructions. That's why I would like to remove the Points from the Instructions :).

roughly n²/2 updates to the PathDetail object

No, as I said, we don't need to propagate the changes. Everything works with this code commented out. At first I thought it would be necessary though.

The only alternative would be to sort the PathDetails by first index, create a dependency graph and then being to only update the PathDetails that are necessary

Instead of doing the simplififcation per PathDetails I tried to do it one run, which makes the process a bit more complicated but saves us a lot of computation (I think).

karussell · 2017-08-14T13:26:49Z

There is stilll a problem in the simplification (I think it is not sufficient to call reduceLength only for cases with removed>0). I've added a general check and in GraphHopperServletIT there will be e.g. this problem:

java.lang.IllegalStateException: PathDetail list average_speed is inconsistent due to entries 60.0 [30, 32] vs. 50.0 [38, 39]
	at com.graphhopper.util.PathSimplification.simplify(PathSimplification.java:135)

It is a bit strange that when you try this in the Measurement class against Germany with my minor changes there will be no cases for this problem.

boldtrn · 2017-08-15T03:26:54Z

Thanks for finding this. It seems that this issue is fixed by uncommenting the loop which I thought that it would be unecessary.

Apparently, the loop that I thought would not be needed is necessary :). When I uncomment the code, the glitch is fixed. I also found another issue, when a connection was not found, we should not calculate the PathDetails. Now the measurement for Baden-Württemberg and Bayern works.

karussell · 2017-08-15T07:57:36Z

core/src/main/java/com/graphhopper/util/PathSimplification.java

-                            /*
-                            // TODO This is not needed, I left it here temporarily, has to be removed before merging
-                            if(listsToSimplify.get(i).get(0) instanceof PathDetail){
+                            if (listsToSimplify.get(i).get(0) instanceof PathDetail) {


Why should it be necessary only for PathDetails?

Ah, because for the remaining PathDetails we need to update the indices and instructions have the pointlist stored and do not need an update if the length changes

Yes exactly 👍

karussell · 2017-08-15T08:35:59Z

core/src/main/java/com/graphhopper/routing/Path.java

@@ -377,7 +377,7 @@ public InstructionList calcInstructions(final Translation tr) {
     * @return List of PathDetails for this Path
     */
    public Map<String, List<PathDetail>> calcDetails(final PathDetailsBuilderFactory pathBuilderFactory, int previousIndex) {
-        if (pathBuilderFactory == null)
+        if (!isFound() || edgeIds.isEmpty() || pathBuilderFactory == null)


Hmmh, we should add an integration test and not calling PathMerger at all or wdyt?

Also understand this now ;) and will add an integration test.

I think the edgeIds.isEmpty() should not be here as a PathDetail could still be included even if the path has no length. So we have to do similar things that we do in calcInstructions when the path is empty but the finish instruction should be there.

karussell · 2017-08-15T08:43:15Z

Thanks!

Another minor thing I noticed is that maxspeed is used as OSM tag and not max_speed. So either we break OSM style for the path details naming (which I would slightly prefer) or we use the ugly averagespeed instead of average_speed now.

boldtrn · 2017-08-16T01:17:40Z

core/src/main/java/com/graphhopper/util/PathSimplification.java

-                            if (listsToSimplify.get(i).get(0) instanceof PathDetail) {
-                                List<PathDetail> pathDetails = listsToSimplify.get(i);
-                                for (int j = offset[i] + 1; j < pathDetails.size(); j++) {
+                            List<PathDetail> pathDetails = listsToSimplify.get(i);


This looks wrong, it can be either InstructionList or a list of PathDetail. It works, but I think we should not leave it like that. WDYT?

boldtrn · 2017-08-16T01:40:19Z

Another minor thing I noticed is that maxspeed is used as OSM tag and not max_speed. So either we break OSM style for the path details naming (which I would slightly prefer) or we use the ugly averagespeed instead of average_speed now.

Mhm, hard to say. For osm users maxspeed makes probably more sense. For everyone else max_speed makes probably more sense. My first intuition would be to go with max_speed, as this is the way we did it with essentially every other parameter, but I don't know a strong argument for either option.

karussell · 2017-08-16T08:54:20Z

core/src/main/java/com/graphhopper/util/PathSimplification.java

                            if (pathDetails.get(0) instanceof PathDetail) {
                                for (int j = offsets[i] + 1; j < pathDetails.size(); j++) {
-                                    PathDetail pd = pathDetails.get(j);
+                                    PathDetail pd = (PathDetail) pathDetails.get(j);


ups, thanks!

karussell · 2017-08-16T17:04:38Z

Thanks @boldtrn for all the work - this is a nice new feature!

First proposal to add path details

d38aa5c

boldtrn added the improvement label Jun 17, 2017

boldtrn self-assigned this Jun 17, 2017

boldtrn requested a review from karussell June 17, 2017 13:23

devemux86 reviewed Jun 17, 2017

View reviewed changes

boldtrn added 2 commits June 27, 2017 17:35

Use number of points to determin the interval for pathdetails

887cbf4

Improved JS debugging code

f10da8a

boldtrn added 3 commits July 1, 2017 13:40

Changed the way PathDetails are stored to a list instead of a Map

6082c39

Merge remote-tracking branch 'remotes/gh/master' into issue_439

b16205f

Store pathdetails with number of points, not intervals

3d87388

karussell requested changes Jul 3, 2017

View reviewed changes

boldtrn added 3 commits July 4, 2017 09:15

Store pathdetails in a map and let jackson handle the serialization

dfa6e34

Merge remote-tracking branch 'remotes/gh/master' into issue_439

e281e9b

Updated according to review comments

75660e9

boldtrn added 5 commits July 4, 2017 19:28

Fixed Via Point Issue

7b95358

Added PathSimplification for Instructions and PathDetails

66754f5

Changed instructions marker to red so it's easier to debug

b72e5c1

Removed temporary disabled simplification

e722752

Changed the way we call DP simplify

3153e10

karussell reviewed Aug 11, 2017

View reviewed changes

boldtrn commented Aug 14, 2017

View reviewed changes

boldtrn added 2 commits August 14, 2017 16:54

Fixed the simplification

618967f

Minor test improvement

009c887

boldtrn commented Aug 14, 2017

View reviewed changes

karussell and others added 3 commits August 14, 2017 15:27

show simplification problem

db89861

show glitch in Measurement

bbbd403

Fixed tests and glitch

f398ab2

karussell reviewed Aug 15, 2017

View reviewed changes

added integration test

871c675

boldtrn commented Aug 16, 2017

View reviewed changes

Changed List details

0894a03

karussell reviewed Aug 16, 2017

View reviewed changes

revert commenting out

e96cf3e

karussell approved these changes Aug 16, 2017

View reviewed changes

Merge branch 'master' into issue_439

89ea614

karussell merged commit 11efb7a into graphhopper:master Aug 16, 2017

karussell added new feature and removed improvement labels Aug 16, 2017

sebalis mentioned this pull request Aug 16, 2017

Add speed, time and more informations to instructions #439

Closed

boldtrn mentioned this pull request Aug 18, 2017

Requests with #Points > 2, duplicated points at waypoints #1139

Open

karussell mentioned this pull request Oct 3, 2017

Improve PathSimplification Performance and Memory Overhead #1171

Merged

boldtrn deleted the issue_439 branch October 6, 2017 01:00

Add Average Speed as PathDetails to Response #1091

Add Average Speed as PathDetails to Response #1091

Conversation

boldtrn commented Jun 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell commented Jun 17, 2017

karussell commented Jun 17, 2017

boldtrn commented Jun 17, 2017

karussell commented Jun 17, 2017

boldtrn commented Jun 17, 2017

karussell commented Jun 17, 2017

boldtrn commented Jun 27, 2017

boldtrn commented Jul 1, 2017

karussell commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boldtrn commented Jul 4, 2017

boldtrn commented Jul 11, 2017

karussell commented Aug 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell commented Aug 14, 2017 • edited

boldtrn commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karussell commented Aug 15, 2017

Choose a reason for hiding this comment

boldtrn commented Aug 16, 2017

Choose a reason for hiding this comment

karussell commented Aug 16, 2017

boldtrn commented Jun 17, 2017 •

edited

karussell Aug 14, 2017 •

edited

karussell Aug 14, 2017 •

edited

karussell commented Aug 14, 2017 •

edited