Multiple origin and destination candidates results in strange detours in unidirectional A* #3585

derolf · 2022-03-29T12:57:22Z

Added tests for multiple origin and destination candidates to reproduce the issue -> tests fail
Fixed unidirectional A* to properly handle these cases -> tests pass

Issue

#3582
#1952

Tasklist

Add tests
Add #fixes with the issue number that this PR addresses
Update the docs with any new request parameters or changes to behavior described
Update the changelog
If you made changes to the lua files, update the taginfo too.

- Adding tests for mutlipe origin and destination candidates - Fixing unidirectional A*

derolf · 2022-03-29T13:19:51Z

Dear reviewers,

the basic idea is to wrap label creation into a lambda (add_label) in both SetOrigin and ExpandInner. add_label is always called to at least add the "normal" edge. If the edge is also a destination, add_label is called for each correlation on that destination, adjusting costs.

Cheers

Daniel

derolf · 2022-03-30T09:51:41Z

The test Standalone, NotThruPruning fails. I looked through my changes and can't figure out what's wrong there. Any idea?

EDIT: My fault, found the reason...

CHANGELOG.md

kevinkreiser · 2022-04-01T13:54:54Z

valhalla/sif/edgelabel.h

+  }
+
+  /**
+   * Sets this edge as an origin.


nit: origin seems like the wrong word maybe? maybe you meant that it can be either and origin or destination since we have both forward and reverse search algorithms?

An edge can be both origin and destination at the same time.

btw "Sets this edge as an origin." is already part of the untouched code, see here:

valhalla/valhalla/sif/edgelabel.h

Line 267 in a5ea654

/**

sorry im just pointing out that the comment says "set this edge as an origin" but the code sets the destination bit. that seems contradictory though yes i understand that its possible an edge is both a destination and an origin for certain routes. really i just wonder why the comment isnt "sets this edge as a destination"

Ah, my bad!!!! Sorry

kevinkreiser · 2022-04-01T13:57:15Z

@dgearhart it looks like the pencilpoint tests are failing on this one with finding completely different routes? i just looked at ci and saw the logs and thats what it looked like. do you have a second to take a look? otherwise i can look maybe in a couple of days.

@derolf thanks very much for the massive refactor of these algorithms. i'm hoping that @dnesbitt61 and i can take a look in the next few days. anyone else who has time (maybe @purew ? ) would also be much appreciated!

dgearhart · 2022-04-01T21:58:17Z

@dgearhart it looks like the pencilpoint tests are failing on this one with finding completely different routes? i just looked at ci and saw the logs and thats what it looked like. do you have a second to take a look? otherwise i can look maybe in a couple of days.

@kevinkreiser will not have any time today

kevinkreiser · 2022-07-28T17:00:09Z

@derolf apologies its taken so long to get to this, but i just wanted to say its still on my radar. just have to appease the tests at this point.. bear with us

derolf · 2022-11-07T04:20:58Z

Hey @kevinkreiser,

Any idea how to fix the tests?

Assuming I would fix them in the next days, would it be ready to get merged?

cheers

Daniel

kevinkreiser · 2022-11-07T07:48:00Z

@derolf we were just starting to look at them last week to see what is going on. As soon as I can qualify the exact cause I'll post back to this issue. All I see at the moment is it takes a different path. It might be that the test itself should change

…not added

derolf · 2022-11-18T17:54:00Z

@nilsnolde @kevinkreiser Coverage went down a tiny bit. Any idea for an easy gain here?

nilsnolde · 2022-11-18T18:01:39Z

Ah that's fine @derolf. Can happen with bigger refactors where lots of (apparently very well tested) code is removed. As for me, the more important thing is the patch diff and that looks as good as it can be.

Though I have a hard time wrapping my head around the overall decrease if the patch diff is so high. I could understand if the patch diff was like 84.9 or so, then I could reason that code was removed that was covered at a much higher degree. But with 96% coverage.. Anyways, maybe I understand those metrics wrong. Again, it's fine with me at least.

derolf · 2022-11-18T18:08:30Z

@nilsnolde Ship it!

nilsnolde · 2022-11-18T18:13:33Z

jep, we'll review it again in a bit. I'm pretty sure it's good though! was just saying it's fine to decrease coverage. thanks again and sorry for the long wait.

nilsnolde · 2022-12-26T16:04:26Z

src/thor/unidirectional_astar.cc

+    assert(opp_edge_id);
+    assert(opp_edge);


if we have the tile, it's not possible that we don't have the edge

nilsnolde

Finally had some time to look into this. Looks good, thanks @derolf . I think the assert are redundant, other than that all good. Will go over it with @kevinkreiser tmrw, then we can merge.

nilsnolde · 2022-12-26T16:54:38Z

src/thor/unidirectional_astar.cc

+    if (FORWARD ? costing_->AvoidAsDestinationEdge(edgeid, edge.percent_along())
+                : costing_->AvoidAsOriginEdge(edgeid, edge.percent_along())) {


I like the changes in this function:)

nilsnolde · 2022-12-26T16:57:22Z

src/thor/unidirectional_astar.cc

+    // edge (check has to be done BEFORE we invert the edge if !FORWARD)
+    if (FORWARD ? costing_->AvoidAsOriginEdge(edgeid, percent_along)
+                : costing_->AvoidAsDestinationEdge(edgeid, percent_along)) {
+      continue;
    }


Hm, not so convinced anymore after all.. For reverse this might not do what we'd expect I think. Basically it seems this relies on the edge as it's being traversed by the algorithm, not its opposing. That exact edge might not even be in the avoid_edges set if the current mode isn't allowed on it. However, its opposing (the one we actually care about) might've been. That is to say, it's better than before, where we didn't look at avoided edges at all for the reverse tree, but might still need some more fiddling to get it really right, so we should leave a TODO.

derolf · 2022-12-27T12:09:15Z

Thanks for approving the PR. What would you like me to do before you merge the PR?

kevinkreiser · 2022-12-27T12:50:13Z

I'd still like to test the pr. We need to run a suite of routes through it and look at the differences vs master. I'll do that and report back. I'd also like to reason about the changes myself i just haven't had the time. I started looking weeks ago but other things came up.

nilsnolde · 2022-12-27T14:27:28Z

Maybe we can do that tmrw together if there’s enough time.

kevinkreiser · 2022-12-28T12:41:28Z

valhalla/thor/unidirectional_astar.h

-  // Destinations, id and percent used along the edge
-  std::unordered_map<uint64_t, float> destinations_percent_along_;
+  // Mark if edge is a destination
+  std::unordered_multimap<valhalla::baldr::GraphId, std::reference_wrapper<const valhalla::PathEdge>>


im not too sure that we need a multimap here. the correlated edges can only have one copy of each unique edge id so you wont ever push in the same edge twice as a destination. unless im missing something here.

That's not correct. The map-matcher could match the same edge multiple times. Just think about a hard loop shaped like a U with the car between.

while it is true that map matching needs to handle this case, this algorithm has nothing to do with our map matching implementation. all our map matching code is in the meili namespace (and a little bit of it in thor/map_matcher thor/trace_*). this is because we do a one-to-many routing between map matching candidates and this algorithm only does a one-to-one routing.

I mean if the incoming destination set has the same edge multiple times in the list, then we would need to add complex logic for tie breaking. Using a multimap usually nicely handles that case without any other checks required.

yep sorry what im saying is that that cannot happen. the list is populated by this line:

valhalla/src/thor/unidirectional_astar.cc

Line 822 in 4532f7b

for (const auto& edge : dest.correlation().edges()) {

that list in turn is populated by loki search and im just telling you that loki search will only have unique edge ids in the list. this is because the whole job of loki search is to find the closest point on a given edge and return that as a candidate. there can be only 1 closest point on a given edge per location. so im just saying that multimap is an unneeded complication. i can see you are using it to nicely handle the event that there would be the same id more than once but what im saying is that there will never be.

One should not couple implementation details of two algorithms, esp. across public APIs.

So relying on the fact that LOKI behaves that way is not good because the A* could also be used in a different way.

one should probably also not constantly change the interface to those public APIs but i guess it depends on the guarantees one wants to provide with a given API.

since its inception the project has not really concerned itself with maintaining compatibility (commit to commit) between the APIs exposed in the headers. we pretty much only gaurantee the json API. in fact it is a long term goal to vastly reduce the scope of methods exposed in the library headers (a battle for another day.. or maybe slowly plugging away at it rather).

furthermore, i can tell you for a fact that other parts of thor assume that there are no duplicates in the list of a locations edge candidates (eg: https://github.com/valhalla/valhalla/blob/master/src/thor/triplegbuilder.cc#L411 if there were duplicates here it would randomly pick one candidate, not necessarily the right one)

anyway the whole reason i was telling you this was because i saw the multimap and i saw the call to equal_range and i was wondering: why is he protecting against this case that doesn't occur? which lead to me really just trying to do a polite: FYI you dont need to worry about this, it cant happen. but i think from your round about answers afterward i could understand that you may be calling this algorithm with your own set of edge candidates which may indeed have duplicates?

my point is i understand how one could put more than one correlation point for the same edge in the list, it just doesnt happen in any of the code here on github. if you have other designs elsewhere or future plans, best to just say that and shortcut the discussion 😄

Nay, no plans right now, but could happen.

I did that change for the simple reason of my edge-cases-tuned brain :-)

kevinkreiser · 2022-12-28T13:07:03Z

valhalla/sif/edgelabel.h

@@ -441,6 +457,7 @@ class EdgeLabel {
  uint64_t shortcut_ : 1;
  uint64_t dest_only_ : 1;
  uint64_t origin_ : 1;
+  uint64_t destination_ : 1;


this is convenient good call

kevinkreiser · 2022-12-28T13:27:15Z

so we ran master and this branch on a 14k route file and the diff results were good for depart at. only 351 diffs total. we will run arrive by and also look at differences in performance. i'll post some of the screen shots of the differences so we can see what kinds of diffs we have. the routes i ran were with a 10m radius so that the logic to select different destination candidates would be triggered. looking good so far! thanks again for all the work @derolf

kevinkreiser · 2022-12-31T03:45:11Z

ok i've checked performance and as expected there is no impact (we are doing the same amount of unordered::finds as we were before basically and the lambda is basically free).

@nilsnolde and i looked at a bunch of the diffs and just about all of them make sense and are better results. there are a few where i'd say the results are worse and not intuitive but on the whole the ones we checked (maybe 20 manually reviewed) id say only a couple were noticeably worse.

algorithmically the change makes sense to me so i think we are good to merge.

derolf force-pushed the master branch from 12dbda3 to 13c30b7 Compare March 29, 2022 12:59

kevinkreiser requested review from kevinkreiser and dnesbitt61 March 29, 2022 13:01

3582: Multiple origin and destination candidates

f07925b

- Adding tests for mutlipe origin and destination candidates - Fixing unidirectional A*

derolf force-pushed the master branch from 13c30b7 to f07925b Compare March 29, 2022 13:03

derolf added 2 commits March 29, 2022 15:04

Merge branch 'master' of https://github.com/valhalla/valhalla into HEAD

5ebb0b0

Fixed CHANGELOG.md

a46e96a

derolf force-pushed the master branch from e63ccd1 to a46e96a Compare March 29, 2022 13:07

derolf changed the title ~~3582: Multiple origin and destination candidates~~ Multiple origin and destination candidates results in strange detours in unidirectional A* Mar 29, 2022

derolf added 2 commits March 29, 2022 15:38

Lint fixes

fc1362d

Added #fixes

8d6782b

derolf added 2 commits March 30, 2022 12:00

Merge branch 'master' of github.com:valhalla/valhalla into HEAD

bd9df64

Fixing cost computation

b1c8203

kevinkreiser reviewed Apr 1, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

kevinkreiser reviewed Apr 1, 2022

View reviewed changes

kevinkreiser added 4 commits April 28, 2022 09:42

Merge branch 'master' into master

63526b4

Merge branch 'master' into master

9ef430f

Merge branch 'master' into master

bf0ef8b

Merge branch 'master' into master

d7930fc

nilsnolde mentioned this pull request Oct 2, 2022

Open PRs summary #3775

Open

Merge branch 'master' into master

bcfd971

kevinkreiser and others added 2 commits November 7, 2022 02:48

Merge branch 'master' into master

9fbf7b6

Bugxfix: If end edge was temporarily labeled, destination edges were …

babc2c3

…not added

Fixing test

d527b2d

nilsnolde self-requested a review November 18, 2022 18:11

derolf force-pushed the master branch from cfae72a to a7b35fe Compare November 21, 2022 11:24

Coverage fix

4909ad8

derolf force-pushed the master branch from a7b35fe to 4909ad8 Compare November 21, 2022 12:56

kevinkreiser self-requested a review December 4, 2022 00:12

Merge branch 'master' into master

aae03bb

nilsnolde mentioned this pull request Dec 19, 2022

Release 3.2.1? #3870

Closed

nilsnolde reviewed Dec 26, 2022

View reviewed changes

nilsnolde previously approved these changes Dec 26, 2022

View reviewed changes

Merge branch 'master' into master

4532f7b

nilsnolde dismissed their stale review via 4532f7b December 26, 2022 17:27

nilsnolde self-requested a review December 27, 2022 10:37

nilsnolde approved these changes Dec 27, 2022

View reviewed changes

kevinkreiser reviewed Dec 28, 2022

View reviewed changes

kevinkreiser mentioned this pull request Dec 30, 2022

update locales #3880

Merged

kevinkreiser approved these changes Dec 31, 2022

View reviewed changes

kevinkreiser merged commit 29f60c5 into valhalla:master Dec 31, 2022

This was referenced Jan 6, 2023

Unidirectional A* variants have issues with multiple destination edges #3582

Closed

Why does increasing loki radius end up giving sub-optimal routes? #2587

Open

		if (FORWARD ? costing_->AvoidAsDestinationEdge(edgeid, edge.percent_along())
		: costing_->AvoidAsOriginEdge(edgeid, edge.percent_along())) {

Multiple origin and destination candidates results in strange detours in unidirectional A* #3585

Multiple origin and destination candidates results in strange detours in unidirectional A* #3585

Conversation

derolf commented Mar 29, 2022 • edited

Issue

Tasklist

derolf commented Mar 29, 2022

derolf commented Mar 30, 2022 • 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

Choose a reason for hiding this comment

kevinkreiser commented Apr 1, 2022

dgearhart commented Apr 1, 2022

kevinkreiser commented Jul 28, 2022

derolf commented Nov 7, 2022

kevinkreiser commented Nov 7, 2022

derolf commented Nov 18, 2022

nilsnolde commented Nov 18, 2022

derolf commented Nov 18, 2022

nilsnolde commented Nov 18, 2022

Choose a reason for hiding this comment

nilsnolde left a 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

derolf commented Dec 27, 2022

kevinkreiser commented Dec 27, 2022

nilsnolde commented Dec 27, 2022

kevinkreiser Dec 28, 2022 • 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

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

kevinkreiser commented Dec 28, 2022

kevinkreiser commented Dec 31, 2022

derolf commented Mar 29, 2022 •

edited

derolf commented Mar 30, 2022 •

edited

kevinkreiser Dec 28, 2022 •

edited