Dedupe query results #91
Conversation
| public List<QueryResult> findNClosest(final double queryLat, final double queryLon, final EdgeFilter edgeFilter, | ||
| double gpxAccuracyInMetern) { | ||
| /** | ||
| * Returns all edges whose starting points are within the specified radius around the |
There was a problem hiding this comment.
@karussell: Can you please check if this javadoc comment is correct?
There was a problem hiding this comment.
I'd reword it to make it clearer that there's no (general) guarantee it finds all such edges (due to searching only nine tiles).
There was a problem hiding this comment.
There is no guarantee if the distance is larger than the cell radius which is usually not the case
There was a problem hiding this comment.
Instead of writing "Returns all edges whose starting points are within the specified radius ..." is it correct to write "Returns all edges that are within the specified radius ..."?
There was a problem hiding this comment.
Ah, you mean this part. Yes, this version would be better.
| } | ||
| return gpxEntryLocations; | ||
| } | ||
|
|
||
| private List<Collection<QueryResult>> dedupeQueryResultsByClosestNode( | ||
| List<Collection<QueryResult>> queriesPerEntry) { | ||
| final List<Collection<QueryResult>> result = new ArrayList<>(); |
There was a problem hiding this comment.
Here an init would be good: new ArrayList<>(queriesPerEntry.size())
| List<Collection<QueryResult>> queriesPerEntry) { | ||
| final List<Collection<QueryResult>> result = new ArrayList<>(); | ||
|
|
||
| final Comparator<QueryResult> dedupeComparator = new Comparator<QueryResult>() { |
There was a problem hiding this comment.
Instead of comparator and TreeSet can we use a HashMap<closest node, query result>? (less memory intense as array based)
|
I added a commit with the changes discussed above. I will squash this with the previous commit when the PR is approved. |
| // QueryResult nodes with virtual nodes. Virtual nodes are not deduped since there is at | ||
| // most one QueryResult per edge and virtual nodes are inserted into the middle of an edge. | ||
| // Reducing the number of QueryResults improves performance since less shortest/fastest | ||
| // routes need to be computed. |
There was a problem hiding this comment.
Maybe you add a "See #91" here?
There was a problem hiding this comment.
Since this PR does not contain additional information I don't see the benefit of this. But I will do this if you generally prefer linking to PRs from the code.
There was a problem hiding this comment.
Just the number "#91" no link?
There was a problem hiding this comment.
Thanks - please feel free to merge or squash (hope this works :)) ... BTW: have now enabled both again as often it is very convenient to avoid manual squashing. But will deactivate it as soon as we see this does not work well somehow
stefanholder
force-pushed
the
dedupe_QueryResults
branch
from
8b87104
to
f4d6a71
Compare
|
Squashed commits. |
stefanholder
force-pushed
the
dedupe_QueryResults
branch
from
f4d6a71
to
10117c7
Compare
|
Referenced PR in commit messages. |
|
Thanks - do you want to merge it or prefer that I do it? |
It's probably cleaner if a reviewer does the merge. |
Good point! |
Dedupes the QueryResults of each GPX entry by node id after calling
QueryGraph.lookupas discussed in #81. This only dedupes tower nodes but not virtual nodes since there is at most one QueryResult per edge and virtual nodes are inserted into the middle of an edge.Reducing the number of QueryResults improves performance since less shortest/fastest routes need to be computed.
Also updated README.md.