WCI refactor

[admrtin]

Draft PR for:

• Walk Comfort Index (WCI) Script Refactor #15

Summary:

• The new point of entry for all OSM network modification types is now WayRTreeEntry, which implements the needed Rtree traits.

TODOs:

• Refactor wci computation script (way_attributes_for_wci.rs)
• Write up some unit tests.

[admrtin]

Hi @robfitzgerald,

This is a pretty big commit (which is ready for a review), so if it makes more sense for us to go through it in person or during a meeting that works with me. I mainly want to make sure that I'm on the right track and that there aren't any blatant errors in how I am refactoring the code.

I discovered a significant bug in the WCI comptuation - previously, the wci scoring mechanism was weighing neighboring links (for both cycle score and speed limit score) by giving more distant links higher weight than nearby links.

Overall, this is starting to look better, but I'm happy to hear any feedback you might have!

[admrtin]

In routee-compass, I saw that custom types to put into an RTree were named as (for example) MapEdgeRTreeObject. Do we want to change WayRTreeEntry to WayRTreeObject?

[robfitzgerald]

In routee-compass, I saw that custom types to put into an RTree were named as (for example) MapEdgeRTreeObject. Do we want to change WayRTreeEntry to WayRTreeObject?

eh, no this is fine. i feel bullied by the API for coming up with an arbitrary naming suffix here in this case, and the suffix Object is about as generic as it gets. honestly, looking at it now, Node seems like a more fitting suffix but Entry makes sense to me too. go for it.

[admrtin]

A first sanity check, on San Francisco's WCI values:

(main vs. refactored)

Something is not right, and I believe it is using the inverse distance for finding the nearest neighbor. I will modify and continue to post my results.

[robfitzgerald]

Something is not right

Sure, big difference there. But before we assume something is not right, I am wondering if mapping something smaller, closer to.. let's say golden gate park, might give us a clearer picture of a sanity check. What do you think?

[admrtin]

The scores are now identical between the old and refactored implementations. I found and fixed several issues:

• Walkways with no adjacent roads were not being assigned the maximum WCI score as intended.
• The traffic speed limit score calculation during nearest-neighbor lookup was incorrect. I was weighting and averaging the individual speed scores of neighboring roads rather than averaging the neighbors' actual speed limits first and then converting the result into a score.

(main vs. refactored, sf + oakland at scale)

(main vs. refactored, near the golden gate bridge)

[admrtin]

I also introduced two new scripts in the script/ folder:

• One script merges the WCI total and component scores with an edges-complete.csv file.
• The other visualizes the WCI total/component scores (from the WCI-merged edges-complete.csv) alongside their associated edges (as shown in the images above).

Here is an example of the resulting tooltip:

[robfitzgerald]

The scores are now identical between the old and refactored implementations. I found and fixed several issues..

YESSSSSSSSSSS Excited!

On my phone, so I can't tell exactly how you set up a plotting CLI workflow without introducing a new lib. Is it that we output something that's instantly ready for geo python, like a geojson or gpkg, etc?

[admrtin]

@robfitzgerald It's nothing too crazy, just through a geopandas GeoDataFrame:

GeoDataFrame.explore()

def create_wci_map(
    gdf: gpd.GeoDataFrame,
    output: Path,
    title: str,
) -> None:
    """Create and save an interactive WCI map."""
    m = gdf.explore(
        column="wci_total",
        cmap="viridis_r",
        tiles="CartoDB positron",
        legend=True,
        tooltip=[
            "name",
            "highway",
            "wci_total",
            "wci_walk",
            "wci_speed",
            "wci_cycle",
            "wci_signal"
        ],
    )

    m.save(output)
    print(f"Saved {output}")
    ```

[admrtin]

@robfitzgerald This PR is ready for a review. I added four unit tests in compute_wci for ways that should recieve:

1. the minimum WCI score (unwalkable way)
2. the maximum WCI score (a path/footway separated from any other ways)
3. a positive WCI score (indicating some walkability)
4. minimum WCI score (indicating not much walkability)

[robfitzgerald]

just read through the test cases and they're all very helpful. wondering though if we want at least one test case that deals in the spatial constraints imposed by the rtree logic.

can we place two ways in the rtree, one that's a footway, one that's a motorway, and place them close enough in space to each other that WCI for the footway would be lower than if it were by itself? to achieve that, we really just need to create let way_2: OsmWayDataSerializable and place it in the RTree we are building? if that would just take 15m i think it would be a good test to add. in fact, here's a mocked up test case:

edge under test

{
    "osmid": 314159,
    "src_vertex_id": 2,
    "dst_vertex_id": 3,
    "highway": "footway",
    "LINESTRING (-105.16845 39.773104, -105.164781 39.773632)",
    "length_meters": 400.0
}

neighbor edge in rtree that is not-so-comfy

{
    "osmid": 42,
    "src_vertex_id": 0,
    "dst_vertex_id": 1,
    "highway": "primary",
    "maxspeed": "45 mph",
    "linestring": "LINESTRING (-105.170016 39.773648, -105.165381 39.774176)",
    "length_meters": 400.0
}

and our required source vertex:

{
    "osmid": 0,
    "x": -105.170016,
    "y": 39.773648,
    "highway": "stop"
}

with not-so-comfy edge in rtree, above footpath as entry, we call compute_wci(&rtree, &entry, &src) and get something worse than expected for a footpath?

[admrtin]

@robfitzgerald This could work, but not for that test case. In the previous impl footways and sidewalks with no neighbors are set to the max score before any neighbor contribution to the overall score:

 else if way_has_footway(&entry.way)
        || (neighboring_ways.is_empty() && way_has_sidewalk(&entry.way))
    {
        // Max WCI score (walking path away from roads)
        WCIComponentScores {
            total_score: MAX_WCI_SCORE,
            walk_score: None,
            traffic_speed_score: None,
            cycle_score: None,
            traffic_signal_score: None,
        }

I was thinking through this, and I could probably do a case for querying a residential road with no posted speed limit or cycleway, being buffed by neighboring cycleways and low speed limit roads...

TLDR we currently don't have a way to inform the WCI score of the queried way by the walk comfort of neighboring ways, only the cycleway type and speed limits of neighboring ways, which are only two of four of the components that go into the overall score.

Let me know what you think!

[admrtin]

@robfitzgerald I added one unit test to account for neighbor contribution to the WCI score

[robfitzgerald]

@robfitzgerald I added one unit test to account for neighbor contribution to the WCI score

// This neighbor has a cycleway, and a low speed limit, so it's
// weighted score should contribute positively to the query's score

thank you for bringing in my request! but i think i got my head flipped around here looking at the test case. it looks like the new test case does in fact show a change to the WCI when we add a neighboring link. however, the result is counterintuitive for me. we have a 45mph road that we are assigning WCI values to. it's a 45mph road, so it should have a bad WCI. but we put a road with a cycleway near it that is slower, and, it improves the WCI for this road? that seems strange to me. most important, i think this flexes the capability we wanted to link into the test suite, so i should be satisfied, but, isn't it weird that we can improve a road that is not comfortable in this way? 🤔

regardless, this is definitely ready for a review, thanks for doing all the work!

[admrtin]

@robfitzgerald Thanks! And yes, this is a good point, I agree it feels weird. Technically, what ends up happening here (in terms of traffic speed) is the traffic speed score stays the same from 45 MPH as before, -1. But because the queried way did not have a bike lane, and the neighbor does have a dedicated lane, the cycle score increases up to the max (+2) and we end up going from a total score of -4 to 0.

Without neighbor:

With neighbor:

In this test case, the neighbor is the only way that is used in the weighted average for cycle score, thus it's contribution is very high.

[robfitzgerald]

good start. thank you for the script adds here. things are definitely more organized and the functionality we desire is working as expected.

halfway through the review, i started getting the feeling that the concern of constructing WCI score values is spread out across a lot of files and really should be centralized into a single module wci_score.rs with a newtype WciScore. what do you think? i think if we did that, some of my nits would unravel naturally as code migrated out of various files. we can chat in our stand-up and decide if this is worth one more set of changes before we accept it.

[robfitzgerald]

looks great! i'm happy with the result, just a small comment but optional, ready to roll it in. thank you for all the effort, @admrtin!

[robfitzgerald]

i love it! looks great :-)