Add table of per-edge factors to network when modification requires it #838
Conversation
this is currently modifying the scenario network in the resolve phase which is probably bad form - ideally we'd shift this to the apply phase.
switch to factory method for EdgeTraversalTimes Create EdgeTraversalTimes in apply phase instead of resolve phase change info message grammar to work with "Modification X" as a header
There was a problem hiding this comment.
This looks good overall, and I agree with the premise.
Is unity descriptive enough? I am less familiar with this area of work, so I may not have the proper context, but method names of
EdgeTraversalTimes.generateNeutralEdges(...) and setAllEdgesToNeutral() might be more appropriate for what they end up doing.
| times.bikeTraversalTimes.setAllUnity(); | ||
| times.walkTraversalTimes.setAllUnity(); |
There was a problem hiding this comment.
Additionally, this section could be replaced with
| times.bikeTraversalTimes.setAllUnity(); | |
| times.walkTraversalTimes.setAllUnity(); | |
| for (int i = 0; i < edgeStore.nEdges(); i++) { | |
| times.bikeTraversalTimes.setOneEdge(); | |
| times.walkTraversalTimes.setOneEdge(); | |
| } |
and the new setAllUnity method could be removed.
There was a problem hiding this comment.
I think I just had a reflex of aiming for better encapsulation ("tell the object for each mode to set its own internal fields") but the EdgeTraversalTimes and SingleModeTraversalTimes are already essentially friend classes. If it's easier to understand without the extra method I'll remove it. I'll rename the existing method though because "setOneEdge" should be something like "addOneNeutralEdge".
I guess this depends on perspective. By "unity" I meant an element that behaves like the number 1, emphasizing that some item scales another item to the same size it already is, rather than being a specific quantity. Applying elements of the type EdgeTraversalTimes to elements of the type EdgeStore effectively transforms the EdgeStore to one with different perceived travel times. There's one specific element of EdgeTraversalTimes that has no effect on any EdgeStore, which is the identity element. I almost used the name "identity" for that reason but "identity" seemed too ambiguous. I guess "unity" is not any better. According to https://en.wikipedia.org/wiki/Identity_element it looks like "neutral element" is a synonym so yes, "neutral" might be more intuitive. Some description of how types behave in terms of algebra can help reason about code and would be helpful for some readers, but that can just be a note in the Javadoc. |
use "neutral" instead of "unity" eliminate nested function calls to improve clarity reuse existing methods to extend with neutral per-edge factors
|
Changes have been made to names and function call nesting as requested. There's also potential to add a test here, of the general functionality of setting generalized costs through modifications, which will indirectly test that the data structure is instantiated when it's missing. I'll re-request review when a test is added. |
Goal was to create a test would have failed before this PR. It tests general effect of applying a modification with a walk cost factor by checking if there were decreased time to reach routed nodes vs a baseline network.
There was a problem hiding this comment.
Thanks for adding the test @trevorgerhardt. This is the kind of thing I had in mind - check the effect of the modification itself on routing, which indirectly checks that the data tables are created. I think there are a couple of changes needed (see comments). I'll push a commit with some suggested fixes for your review.
| var scenario = new Scenario(); | ||
| scenario.modifications = Lists.newArrayList(ms); | ||
|
|
||
| var reachedVertexes = getReachedVertexes(network, new Scenario()); |
There was a problem hiding this comment.
Throughout the R5 codebase we have used "vertices" as the plural of "vertex".
| totalGreater++; | ||
| } | ||
| } | ||
| Assertions.assertEquals(454, totalGreater); |
There was a problem hiding this comment.
Checking against an exact number probably makes the test more brittle with respect to changes in routing. Ideally we'd do more checks that will verify expected outcomes even after unforeseen changes to routing details - like your check that baselineTime >= modTime. The first checks that come to my mind are: when increasing the travel speed, the post-modification results should always reach many more vertices, all reached vertices in pre-modification results should be present in post-modification results, and travel times should all be higher. Given the high speedup factor, all post-modification travel times should be 1/10 of the pre-modification travel times (could relax to "at most 1/3" to make it more flexible).
There was a problem hiding this comment.
I agree, I wasn't totally sure how to structure this test. Thanks for updating that.
|
|
||
| int totalGreater = 0; | ||
| for (int i = 0; i < reachedVertexes.size(); i++) { | ||
| int baselineTime = reachedVertexes.get(i); |
There was a problem hiding this comment.
reachedVertexes is a map from vertex ID to the value of the optimization variable at that vertex. This iteration is using positional indexes in as if they are vertex IDs. Most of the get() calls are probably returning -1 which will be equal in both sets of output.
There was a problem hiding this comment.
My mistake! Thanks for correcting that.
use inequalities for softer assertions style changes, use Arrays.asList()
Some of the options for ModifyStreets modifications adjust generalized costs for walking and biking. These options would only work on special networks containing a special generalized costs data table that is not present by default in most networks. Those data tables are currently only created in networks built from very particular OSM data where every way has all of the special tags contributing to the LADOT generalized cost (com.conveyal.r5.streets.LaDotCostTags).
Some users have tried out the walk cost and bike cost ModifyStreets parameters and seen cryptic messages like "bikeGenCostFactor can only be set to values other than 1 on networks that support per-edge factors." Rather than keeping and repeatedly explaining this disappointing/confusing behavior, I decided to change the modification apply() code to simply create the data table in the scenario copy of the network as needed. This means each scenario will have its own (potentially large) generalized cost data table instead of just extending a shared one in the baseline network. This is a rarely used specialty modification, so such less-than-optimal implementation is acceptable at least as a stopgap. The other alternatives would be:
This works as expected in my own testing.