Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

only allow topological node consolidation to be run once on a graph #1273

Merged
merged 4 commits into from
Jan 25, 2025
Merged

Conversation

gboeing
Copy link
Owner

@gboeing gboeing commented Jan 25, 2025

This PR fixes inscrutable error messages from occurring when trying to run topological node consolidation via the consolidate_intersections more than once on the same graph.

For example, consider the snippet:

import osmnx as ox
G = ox.graph.graph_from_place("Piedmont, California, USA", network_type="drive")
G = ox.projection.project_graph(G)
G = ox.simplification.consolidate_intersections(G, tolerance=10)
G = ox.simplification.consolidate_intersections(G, tolerance=10)

Would give you:

ValueError: Grouper for 'cluster' not 1-dimensional

And if you instead set tolerance=1 you'd get:

TypeError: networkx.classes.digraph.DiGraph.add_node() got multiple values for keyword argument 'osmid_original'

These errors are impossible for a user to understand what's wrong. The nature of the problem is that the node consolidation algorithm can only be run once (because it's lossy) and should only be run once (because its parameterization allows near infinite customization to get a precise outcome in only one run). The simplify_graph function handles this gracefully by marked a graph as "simplified" and then raising an interpretable error if it's run again.

This PR makes the consolidate_intersections consistent with the behavior of the simplify_graph function so its behavior is clearer for users:

  • A graph is marked as "consolidated" with a graph-level attribute if the consolidate_intersections function is run in topological mode (i.e., you rebuilt the graph)
  • If a graph marked as "consolidated" has theconsolidate_intersections function run in topological mode again, it raises an error with a clearer message: "GraphSimplificationError: This graph has already been consolidated, cannot consolidate it again."
  • The io module gracefully handles the "consolidated" graph-level attribute when loading to/from file.
  • Fixes a bug where the consolidate_intersections would mutate the passed-in graph itself.

Copy link

codecov bot commented Jan 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.44%. Comparing base (ce0615b) to head (0786a73).
Report is 5 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1273   +/-   ##
=======================================
  Coverage   98.44%   98.44%           
=======================================
  Files          24       24           
  Lines        2384     2385    +1     
=======================================
+ Hits         2347     2348    +1     
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gboeing gboeing merged commit 5aac9c1 into main Jan 25, 2025
10 checks passed
@gboeing gboeing deleted the simp branch January 25, 2025 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant