Call correct isochrone compute method depending on requested expansion type #2967
Conversation
danpat
force-pushed
the
danpat_isochrone_fix1
branch
from
0a3f63a
to
32c102b
Compare
| @@ -235,10 +235,10 @@ void Dijkstras::Expand(ExpansionType expansion_type, | |||
| Compute(*api.mutable_options()->mutable_locations(), reader, costings, mode); | |||
| break; | |||
| case ExpansionType::reverse: | |||
| Compute(*api.mutable_options()->mutable_locations(), reader, costings, mode); | |||
| ComputeReverse(*api.mutable_options()->mutable_locations(), reader, costings, mode); | |||
There was a problem hiding this comment.
Is it possible to add tests (new or update existing) to catch such issues? (haven't looked at current isochrone tests)
There was a problem hiding this comment.
its hard to do the multimodal one unless we just test that it works for a short pedestrian only route. one day we need to figure out how to generate transit pbfs from gurka that would be awesome.
as for reverse we can totally test it but it will be just as unscientific of a test as the existing isochrone test in that we'll just check that a polygon comes out. its very hard to really accurately test this type of thing where the result is a geometric shape.
|
This is a good breakfix. We should add testing but it seems unfair to put that burden on this PR given there was no testing on it before. |
|
@kevinkreiser 👍 thanks - I have some refactoring PRs coming to start to address #2885, and I'm going to start with aligning the isochrone and bidir_astar code. I'll include some tests with that, because as this PR shows, refactoring can be risky :-) |
Issue
Looks like this was a typo introduced in the refactoring done here
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?