Repository navigation
Conversation
tallcoleman
left a comment
There was a problem hiding this comment.
Hi Kate,
Great addition - I've looked through it and it makes sense to me. I've left one comment that should be resolved before merge about how values from get_member_ref_id are likely to overlap unless both the key and value are stored.
The other thing I'm curious about (but not a blocker) is about the overall approach. I see that you've matched the unclustered data with the clusters that come out downstream using get_member_ref_id. The other option would have been to have each clustering function return a clustered and unclustered gdf output so that the final unclustered dataset goes through the same split-recombine process that the clustered data does. I'm not really sure which solution is more elegant, but I'd be interested to know what your thoughts are about it.
| ] | ||
|
|
||
|
|
||
| def get_member_ref_id(row: pd.Series, ref_columns: list[str]) -> str: |
There was a problem hiding this comment.
Should this store both the key and the value for the ref column? Otherwise you get a potential collision:
ref:open.toronto.ca:bicycle-parking-racks:objectid=10 -> 10
ref:open.toronto.ca:bicycle-parking-high-capacity-outdoor:id=10 -> 10
This may not have come up during testing since all of the City datasets except for street-furniture-bicycle-parking have been fully surveyed into OpenStreetMap. So none of the values from their original datasets are currently making it through to these downstream steps and won't unless the City updates those datasets down the line to include features that haven't been surveyed yet. All of the ids from these other datasets are just integers, unlike street-furniture-bicycle-parking, which has the BP-##### format.
| group_sizes = gdf.groupby(cluster_col)[cluster_col].transform("size") | ||
|
|
||
| cluster_ids_by_label = { | ||
| label: hashlib.md5( |
There was a problem hiding this comment.
Maybe wrap hashlib.md5(...).hexdigest()[:12] in str() for typing purposes? It's showing the output format as dict[Hashable, str] in my IDE.
Summary
Implements #403 (Toronto data path only). Lets the frontend drill into a clustered parking point and see/report on the individual rack it represents, instead of only the merged point.
Every row that got merged into a cluster (2+ members) is now tagged with a
meta_cluster_id, written onto both the merged display row and the original pre-merge rows inopen_toronto_ca_unclustered.geojson. So now the frontend can look up individual rows via a simple tag matchChanges
downstream.py: newget_member_ref_id()(stable per-row id, City ref id with a coordinate fallback) and_assign_cluster_ids()(tags 2+-member groups with their member ids).group_proximate_rings/group_proximate_racksnow return(dissolved_df, member_cluster_ids)instead of just the dissolved frame.update_bicycle_parking.py: propagatesmeta_cluster_idonto rows beforeopen_toronto_ca_unclustered.geojsonis saved.test_downstream.py: new/extended tests covering the tagging logic.