Chapter pedestrian rewrite #257
Conversation
WalkthroughThis PR updates the website workflow build and deployment folder, refreshes pedestrian images and documentation, refactors pedestrian script generation in CMake to a centralized OUTPUT/DEPENDS flow, and migrates pedestrian SQL views/queries to the new walk_net schema with updated names, columns, and cost expressions; localization templates updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Builder as Doc Builder
participant Site as Website
Dev->>GH: Push to main
GH->>Builder: Run make doc
Builder-->>GH: Produce docs artifacts
GH->>Site: Deploy artifacts to folder "3.1"
Note over GH,Site: Deployment folder for main changed from "3.0" → "3.1"
sequenceDiagram
autonumber
participant CMake as CMake
participant Gen as add_custom_command
participant Target as basic_pedestrian_scripts
CMake->>Gen: Declare OUTPUT ${GENERATED_FILES}\nDEPENDS pedestrian.sql, images.sql
Gen-->>CMake: Generated files registered
CMake->>Target: add_custom_target(... DEPENDS ${GENERATED_FILES})
Note over CMake,Target: Centralized generation replaces per-file foreach + BYPRODUCTS/PRE_BUILD
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
docs/basic/images/pedestrian/pedestrian_combinations.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_dijkstracost.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_many_to_many.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_one_to_many.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_one_to_one.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_route1.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_route2.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_route4.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/pedestrian_route5.pngis excluded by!**/*.pngdocs/basic/images/pedestrian/route.pngis excluded by!**/*.png
📒 Files selected for processing (11)
.github/workflows/website.yml(2 hunks)docs/basic/images/pedestrian/CMakeLists.txt(1 hunks)docs/basic/pedestrian.rst(10 hunks)docs/basic/plpgsql_function.rst(1 hunks)docs/basic/sql_function.rst(1 hunks)docs/interactions/chapter-9.rst(1 hunks)docs/scripts/basic/pedestrian/CMakeLists.txt(1 hunks)docs/scripts/basic/pedestrian/images.sql(1 hunks)docs/scripts/basic/pedestrian/pedestrian.sql(4 hunks)locale/en/LC_MESSAGES/basic/pedestrian.po(3 hunks)locale/pot/basic/pedestrian.pot(2 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 (27)
.github/workflows/website.yml (2)
86-86: Doc-only build step looks goodTargeting
make dockeeps this workflow scoped to the documentation artifacts we publish. 👍
98-98: Release folder rename acknowledgedPointing
mainreleases at3.1lines up with the new version structure. Looks right.docs/basic/sql_function.rst (1)
14-14: LGTM!The image reference update to
pedestrian_one_to_one.pngaligns with the PR's goal of using meaningful image names.docs/basic/plpgsql_function.rst (1)
13-13: LGTM!Consistent image rename across documentation files.
docs/interactions/chapter-9.rst (1)
14-14: LGTM!The relative path update maintains consistency with the image renaming across the documentation.
docs/basic/images/pedestrian/CMakeLists.txt (1)
5-11: LGTM!The new image naming convention (e.g.,
pedestrian_one_to_one.png,pedestrian_many_to_many.png) is more descriptive and clearly indicates the routing pattern each image represents.docs/basic/pedestrian.rst (3)
21-24: LGTM!Good addition of the function list at the chapter start, helping readers understand what will be covered.
224-224: Verify cost calculation formula.The description mentions "walking speed
s = 1.3 m/s" and the cost should be in minutes. Ensure this matches the SQL implementation which useslength / 1.3 / 60.
- Time in seconds:
length / 1.3- Time in minutes:
length / 1.3 / 60This appears mathematically correct.
189-192: No action needed: cost setup matches 2 m/s walking speed.
Thewalk_net.costcolumn is defined in graphs.sql aslength_m / 2.0, yielding seconds at 2 m/s.docs/scripts/basic/pedestrian/CMakeLists.txt (2)
5-14: LGTM!The updated file list correctly reflects the new exercise structure with more descriptive names:
get_id.txtone_to_one.txtmany_to_one.txtone_to_many.txtmany_to_many.txtcombinations.txtdijkstracost.txtnote_1.txt
20-27: Improved dependency tracking.The refactored custom command with explicit
OUTPUTandDEPENDSclauses provides better build dependency management compared to the previous approach.docs/scripts/basic/pedestrian/pedestrian.sql (8)
3-5: LGTM!The query correctly retrieves OSM IDs and internal vertex IDs from the vertices table.
9-15: LGTM!Exercise 1: Uses
lengthas cost (meters) fromwalk_net, consistent with the documentation stating "costs in length in meters."
19-25: LGTM!Exercise 2: Correctly converts length to kilometers using
length/1000, matching the documentation requirement.
39-45: LGTM!Exercise 4: The cost calculation
length / 1.3 / 60correctly computes time in minutes for walking speed of 1.3 m/s:
- Time in seconds = length (meters) / 1.3 (m/s)
- Time in minutes = time in seconds / 60
49-57: LGTM!Exercise 5: Uses the same cost calculation as Exercise 4, consistent with the documentation stating the same walking speed.
61-67: LGTM!Exercise 6: Correctly uses
pgr_dijkstraCostwith the same cost expression, providing only aggregated costs as documented.
69-124: LGTM!The note generation correctly uses the same cost expression (
length / 1.3 / 60) to compute and display total times for each route combination, making it easy to verify results.
29-35: walk_net.cost column correctly defined.
The materialized view defines cost as length_m / 2.0, yielding time in seconds at 2 m/s as intended.locale/pot/basic/pedestrian.pot (1)
1-363: LGTM!The POT file has been properly regenerated to reflect the documentation updates. The new translation entries for the pgRouting functions section and updated line references are consistent with the chapter rewrite.
docs/scripts/basic/pedestrian/images.sql (7)
1-12: LGTM!The
starsview correctly migrates fromways_vertices_pgrtoverticesand updates column references (the_geom→geom,gid→id).
14-24: LGTM!The
pedestrian_one_to_oneview correctly demonstrates a single-source, single-destination route usinglengthas cost (in meters). The schema migration towalk_netand updated column names are consistent.
26-36: LGTM!The
pedestrian_many_to_oneview correctly useslength/1000to express cost in kilometers, demonstrating the many-to-one routing pattern.
38-48: LGTM!The
pedestrian_one_to_manyview correctly uses thecostcolumn (in seconds) for the one-to-many routing pattern. This aligns with the documentation that references the costs setup from the graphs chapter.
50-60: LGTM!The
pedestrian_many_to_manyview correctly calculates cost in minutes using the formulalength / 1.3 / 60, where:
lengthis in meters1.3is the walking speed in m/s60converts seconds to minutesThe math is correct: time (minutes) = distance (m) / speed (m/s) / 60 (s/min).
62-74: LGTM!The
pedestrian_combinationsview demonstrates the combinations syntax for specifying specific source-target pairs, using the same cost formula as the many-to-many view. The VALUES clause correctly structures the combinations input.
76-89: LGTM!The
pedestrian_dijkstraCostview correctly demonstratespgr_dijkstraCostusage, which returns only aggregated costs without the detailed path. The creation of line geometries between start and end vertices usingST_MakeLineprovides a visual representation of the cost relationships.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/basic/pedestrian.rst (2)
118-128: Consider refining cost description phrasing.The phrasing "costs in
lengthin meters" (line 118) and "The pedestrian's cost is in terms of length. In this caselength" (line 126) could be clearer:Apply this diff for improved clarity:
-* Calculate routes with costs in ``length`` in meters. +* Calculate routes with length-based costs in meters.-* The pedestrian's cost is in terms of length. In this case ``length`` (line **3**). +* The cost is calculated using the ``length`` attribute in meters (line **3**).
189-190: Standardize unit notation.The unit notation "mts/sec" should be "m/s" for consistency with other exercises (e.g., line 224 uses "m/s").
Apply this diff:
-* Calculate routes using the ``cost`` which is in seconds for a walking speed of - ``2 mts/sec``. +* Calculate routes using the ``cost`` which is in seconds for a walking speed of + ``2 m/s``.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/basic/pedestrian.rst(10 hunks)locale/en/LC_MESSAGES/basic/pedestrian.po(3 hunks)locale/pot/basic/pedestrian.pot(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- locale/en/LC_MESSAGES/basic/pedestrian.po
🔇 Additional comments (6)
locale/pot/basic/pedestrian.pot (1)
1-363: LGTM! Auto-generated localization file.This POT file is auto-generated from the RST source documentation. Any text quality issues here reflect the source file and will be addressed in the review of
docs/basic/pedestrian.rst.docs/basic/pedestrian.rst (5)
13-24: LGTM! Improved navigation.The updated image reference and the new "pgRouting functions in this chapter" section enhance the documentation structure and user navigation.
214-246: LGTM! Consistent documentation.Exercise 4 correctly uses the
pedestrian_many_to_many.pngimage and maintains consistent unit notation ("m/s").
247-277: LGTM! Combinations exercise structure.Exercise 5 correctly demonstrates route combinations with appropriate image reference and cost descriptions.
278-333: LGTM! Effective demonstration of pgr_dijkstraCost.The pgr_dijkstraCost section and Exercise 6 effectively demonstrate the use of this function for obtaining aggregated costs without full path details.
88-93: Verify snippet files exist.The referenced literalinclude files (
docs/scripts/basic/pedestrian/get_id.txt,one_to_one.txt, etc.) were not found in the repo; confirm they exist at the expected path.
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit