conveyal / r5

Developed to power Conveyal's web-based interface for scenario planning and land-use/transport accessibility analysis, R5 is our routing engine for multimodal (transit/bike/walk/car) networks with a particular focus on public transit
https://conveyal.com/learn
MIT License
272 stars 71 forks source link

Revise Simpson Desert tests #896

Closed abyrd closed 8 months ago

abyrd commented 8 months ago

These tests were using Mercator gridded destinations that did not align with the street intersections or transit stops. This added a somewhat unpredictable walking delay to the end of each itinerary. This also caused the tests to fail when changing the representative points of grid pixels where travel times are measured. This PR or similar changes are necessary to make tests pass on PR #894: ideally this PR should be merged first before ones changing anything about Mercator grids or travel time sample points.

While I was working on this I took care of some other cleanup items for these tests. These are the changes:

This PR removes the use of web Mercator grids as destinations, concentrating testing on transit routing. To the extent that we also want the Simpson Desert tests to serve as integration tests of the whole routing system, we will eventually want some tests to use gridded destinations, test that grid cell / pixel sample points are properly situated and introducing the correct amount of walk time. But that should be done in a controlled way, not by inducing unpredictable extra walk times on the end of these more precise tests.

abyrd commented 8 months ago

While debugging a one-minute (board slack?) discrepancy, I noticed that OneOriginResult.traveltimes has 5252 values even though there's only one destination point. Need to investigate.

trevorgerhardt commented 8 months ago

Comments seem to indicate this PR is still being worked on. Is this PR still in progress or ready for a review? @abyrd

abyrd commented 8 months ago

Comments seem to indicate this PR is still being worked on. Is this PR still in progress or ready for a review? @abyrd

I am taking a look at the OneOriginResult.traveltimes with 5252 values because I think it's the only thing really blocking this change, and this PR is needed before we can switch to pixel centers. Hoping to have it cleared up shortly.

The two-minute board slack (?) thing is not a regression, just an unclear explanation of why exactly the travel times are what they are. I haven't yet figured out the origin of this discrepancy but I think it has to do with things being binned into 1-minute bins. I am comfortable leaving it as-is until we have a full explanation.