Graphs chapter rewrite #256
Conversation
WalkthroughReworks the Graphs workshop: adds sphinx-copybutton, reorganizes docs/CMake and image/script build targets, expands and renames graph documentation to a vertices-centric schema (adds tag_id propagation), introduces pgr_dijkstraCostMatrix and images.sql for cost-matrix generation, and updates localization files. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CMake as CMake
participant Data as basic_data_scripts
participant Graphs as basic_graphs_scripts
participant Ped as basic_pedestrian_scripts
participant Veh as basic_vehicles_scripts
participant SQL as basic_sql_function_scripts
Note over CMake: Revised build orchestration for graphs/images/scripts
CMake->>Data: build data.txt
CMake->>Graphs: generate graphs artifacts (graphs.sql, images.sql → generated .txt)
Graphs-->>Ped: satisfy dependency
Graphs-->>Veh: satisfy dependency
Graphs-->>SQL: satisfy dependency (alongside Veh)
sequenceDiagram
autonumber
participant U as User
participant DB as Postgres
participant PGR as pgr_dijkstraCostMatrix
participant VNet as vehicle_net
participant Vtx as vertices
participant Out as costMatrix_png
rect rgba(230,245,255,0.6)
Note over U,DB: images.sql flow (cost matrix generation)
U->>DB: RUN images.sql
DB->>PGR: CALL pgr_dijkstraCostMatrix(SELECT * FROM vehicle_net, [ID array])
PGR-->>DB: returns rows (start_vid, end_vid, agg_cost)
end
DB->>Vtx: JOIN start_vid/end_vid → coordinates
DB->>Out: INSERT gid, ST_MakeLine(start_geom,end_geom), name
Note over Out: Visualization rows produced
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/basic/graphs.rst (1)
128-142: Incorrect function description.Lines 128-130 state that
pgr_extractVerticescomputes connected components using Depth First Search, but this description actually belongs topgr_connectedComponents. The functionpgr_extractVerticesextracts vertices from an edge table.Additionally, Line 142 links to
pgr_connectedComponentsdocumentation when it should link topgr_extractVerticesdocumentation.Apply this diff to fix the description and link:
-``pgr_extractVertices`` compute the connected components of an undirected -graph using a Depth First Search approach. A connected component of an -undirected graph is a set of vertices that are all reachable from each other. +``pgr_extractVertices`` extracts all vertices from an edge table, creating a +vertices table with unique vertex identifiers and their associated properties.Description of the function can be found in `pgr_extractVertices -<https://docs.pgrouting.org/latest/en/pgr_connectedComponents.html>`__ +<https://docs.pgrouting.org/latest/en/pgr_extractVertices.html>`__
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
docs/basic/images/graphs/ch7-e3.pngis excluded by!**/*.pngdocs/basic/images/graphs/costMatrix.pngis excluded by!**/*.pngdocs/basic/images/graphs/pedestrian_only_roads.pngis excluded by!**/*.pngdocs/basic/images/graphs/roads_tag_ids.pngis excluded by!**/*.pngdocs/basic/images/graphs/taxi_net.pngis excluded by!**/*.pngdocs/basic/images/graphs/vehicle_net.pngis excluded by!**/*.pngdocs/basic/images/graphs/walk_net.pngis excluded by!**/*.png
📒 Files selected for processing (14)
REQUIREMENTS.txt(1 hunks)docs/basic/CMakeLists.txt(1 hunks)docs/basic/graphs.rst(23 hunks)docs/basic/images/CMakeLists.txt(1 hunks)docs/basic/images/graphs/CMakeLists.txt(1 hunks)docs/conf.py(1 hunks)docs/index.rst(1 hunks)docs/scripts/basic/CMakeLists.txt(2 hunks)docs/scripts/basic/data/CMakeLists.txt(1 hunks)docs/scripts/basic/graphs/CMakeLists.txt(2 hunks)docs/scripts/basic/graphs/graphs.sql(4 hunks)docs/scripts/basic/graphs/images.sql(1 hunks)locale/en/LC_MESSAGES/basic/graphs.po(3 hunks)locale/pot/basic/graphs.pot(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (13)
docs/basic/graphs.rst (2)
65-124: Well-structured new configuration section.The new "Configuration from osm2pgrouting" section provides valuable context about OSM data types and their usage in routing. The progressive disclosure using collapse blocks and verification rubrics enhances readability.
617-633: Good addition of pgr_dijkstraCostMatrix documentation.The new section properly documents the cost matrix function with signature, description, and proper external documentation links. This aligns well with Exercise 8 below.
docs/index.rst (1)
34-34: LGTM: Graphs chapter correctly repositioned.The reordering places the graphs documentation earlier in the Basic section, which aligns with the PR objective to present graphs first.
locale/pot/basic/graphs.pot (1)
11-11: LGTM: POT file updated to reflect documentation changes.The translation template has been regenerated with the updated POT-Creation-Date and all msgid entries correctly reflect the restructured graphs documentation. As a generated artifact, this file appropriately mirrors the source RST changes.
docs/scripts/basic/graphs/graphs.sql (7)
6-18: Good addition of configuration inspection queries.The new configuration-related queries provide valuable insight into OSM highway types and their usage in the dataset. The output files align with the references in the documentation.
165-182: Verify pgr_dijkstraCostMatrix usage with vertex IDs.The queries pass
ARRAY[@ID_1@, @ID_2@, @ID_3@, @ID_4@, @ID_5@]as vertex identifiers topgr_dijkstraCostMatrix. Confirm that these substitution variables resolve to vertex IDs that exist in the respective net views (vehicle_net, taxi_net, walk_net).According to pgRouting documentation, the vertex array should contain identifiers that match the
sourceandtargetcolumns in the edge SQL.
148-161: Good practice: Creating separate vertex tables for each network.Extracting vertices from each network view (vehicle_net, taxi_net, walk_net) ensures that each routing network has its own consistent vertex set. This is a solid approach for managing different routing graphs.
1-4: Ensure CASCADE drops are intentional.The DROP statements use CASCADE, which will drop dependent objects. Verify this is intentional, especially for the
verticestable which may have dependencies from the net views.
22-26: Verifywaystable schema includes required columns
Ensure thewaystable definessource,target,source_osm, andtarget_osmbefore running this script, as they’re referenced in the UPDATE statements on lines 36 and 41.
36-42: Confirmsource_osmandtarget_osmexist on thewaystable.
The SQL relies on osm2pgrouting’s import to create these columns—please verify your database schema includes them before running these updates.
93-97: Inconsistentsource/targetidentifiers invehicle_net
The documentation (docs/basic/graphs.rst:451) specifies that pgRouting’ssource/targetmust be OSM node IDs, yet the view selects the internalsource, targetcolumns fromways. Either change the SELECT tosource_osm AS source, target_osm AS targetor update the docs to reflect that you’re using internal routing IDs.
docs/basic/images/graphs/CMakeLists.txt (2)
6-12: LGTM: New images added for expanded documentation.The new image files (costMatrix.png, pedestrian_only_roads.png, roads_tag_ids.png, taxi_net.png) align with the expanded graphs chapter content, particularly the new "Configuration from osm2pgrouting" section and cost matrix visualization.
19-19: Good refactor: Using configure_file instead of FILE(COPY).Switching from
FILE(COPY ${f} DESTINATION "${CMAKE_CURRENT_BINARY_DIR}")toconfigure_file(${f} ${f} COPYONLY)is more idiomatic in modern CMake and produces equivalent behavior for copying files.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/basic/graphs.rst(23 hunks)docs/scripts/basic/graphs/images.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/scripts/basic/graphs/images.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
locale/pot/basic/graphs.pot (3)
109-110: Use standard abbreviation for meters per second.The abbreviation "mts/sec" is non-standard. Consider using "m/s" or spelling out "meters per second" for clarity.
Apply this diff to use the standard abbreviation:
-msgid "The walking speed is ``2 mts/sec``." +msgid "The walking speed is ``2 m/s``."
543-545: Use standard abbreviation for meters per second.Same issue as line 109: "mts/sec" should be "m/s" or "meters per second".
Apply this diff:
-msgid "The speed is ``2 mts/sec``." +msgid "The speed is ``2 m/s``."
454-455: Clarify source/target column requirements.The text states "The
sourceandtargetrequirements for the function are to be with OSM identifiers" which may be confusing. If the columns are namedsourceandtarget(notsource_osmandtarget_osm), clarify whether the values are OSM identifiers or internal vertex IDs.Consider rewording for clarity, such as: "The
sourceandtargetcolumns contain vertex identifiers derived from OSM data."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
locale/en/LC_MESSAGES/basic/graphs.po(3 hunks)locale/pot/basic/graphs.pot(3 hunks)
🔇 Additional comments (6)
locale/pot/basic/graphs.pot (6)
1-18: LGTM!The POT file header is correctly formatted with appropriate metadata and the updated creation date.
31-46: LGTM!The pgRouting function references are properly formatted for Sphinx documentation.
286-420: LGTM!The
pgr_connectedComponentssection has the correct function description, proper documentation URL, and well-structured exercises.
574-584: LGTM!The
pgr_dijkstraCostMatrixfunction description and documentation URL are correct.
635-677: LGTM!Exercise 8 provides clear, comprehensive instructions for testing the created views with practical examples.
254-256: No change needed—source_osmremains a valid column storing OSM IDs separate from thesourcevertex ID; the documentation reference is correct.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/basic/graphs.rst(23 hunks)locale/en/LC_MESSAGES/basic/graphs.po(3 hunks)locale/pot/basic/graphs.pot(3 hunks)
1 participant
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
New Features
Documentation
Chores