edgeDisjoint bellman ford use process driver#3090
Open
cvvergara wants to merge 8 commits intopgRouting:developfrom
Open
edgeDisjoint bellman ford use process driver#3090cvvergara wants to merge 8 commits intopgRouting:developfrom
cvvergara wants to merge 8 commits intopgRouting:developfrom
Conversation
Contributor
WalkthroughThis PR consolidates the bellmanFord and edgeDisjoint algorithms to use a centralized shortestPath driver and process mechanism. Dedicated algorithm-specific drivers are removed, replaced with unified C++ function handlers. New enum values (BELLMANFORD, EDGEDISJOINT) are added to the Which enum, and supporting utilities updated to dispatch to the appropriate implementation. The refactoring standardizes how these algorithms integrate with the core routing framework. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
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)
src/cpp_common/utilities.cpp (1)
179-186: 🧹 Nitpick | 🔵 TrivialHandle
EDGEDISJOINTas non-driving-side inestimate_drivingSide.
EDGEDISJOINTcurrently falls intodefault, which can coerce' 'into'b'. For consistency with non-driving-side shortest-path modes, it should return' 'directly.♻️ Proposed change
switch (which) { case ASTAR : case BDASTAR : case DAGSP : case EDWARDMOORE: case BELLMANFORD : + case EDGEDISJOINT: case BDDIJKSTRA: case DIJKSTRA: return ' '; break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cpp_common/utilities.cpp` around lines 179 - 186, The switch in estimate_drivingSide treats EDGEDISJOINT via the default path which can coerce ' ' into 'b'; add EDGEDISJOINT as one of the explicit cases that return ' ' so it is handled as non-driving-side like ASTAR/BDASTAR/DAGSP/EDWARDMOORE/BELLMANFORD/BDDIJKSTRA/DIJKSTRA, i.e. insert "case EDGEDISJOINT:" into the same case group that returns ' ' to ensure consistent behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/c_common/enums.h`:
- Around line 36-37: The get_name(Which) overload in
src/cpp_common/utilities.cpp currently returns "unknown" for BELLMANFORD and
EDGEDISJOINT; locate the get_name(Which) function and either (a) confirm no
reachable call sites pass the new enum values by searching call sites that pass
Which, or (b) add explicit switch cases for BELLMANFORD and EDGEDISJOINT in
get_name(Which) so they map to the proper string names (use the enum identifiers
BELLMANFORD and EDGEDISJOINT to find the exact enum and implement the
corresponding case arms), ensuring the function never falls through to "unknown"
for these values.
In `@src/cpp_common/to_postgres.cpp`:
- Around line 317-326: The loop that assigns costs to path entries (variables
paths, edges, found, r.cost, e.id, e.source, e.cost, e.reverse_cost) can leave
routable rows (r.edge >= 0) unresolved; add a final sanity check after the loop
that scans paths for any entry with r.edge >= 0 whose r.cost is still
unset/invalid and if any are found abort/throw/log a fatal error (e.g., mirror
pgRouting's "No result generated, report this error") so unresolved mappings
cannot silently return stale costs; ensure the check only considers routable
edges (r.edge >= 0) and includes a clear message identifying the inconsistency
before returning tuples.
In `@src/max_flow/maxflow.cpp`:
- Around line 39-43: The functions single_execution and edgeDisjoint currently
take std::vector<Edge_t> by value which causes expensive copies when
edgeDisjoint repeatedly calls single_execution; change both function signatures
to take const std::vector<Edge_t>& edges (and adjust any forward
declarations/overloads) so the vector is passed by const reference. Ensure
inside single_execution you do not mutate edges; if mutation is required, make a
local copy there only. Update all call sites (edgeDisjoint -> single_execution)
to match the new signature.
---
Outside diff comments:
In `@src/cpp_common/utilities.cpp`:
- Around line 179-186: The switch in estimate_drivingSide treats EDGEDISJOINT
via the default path which can coerce ' ' into 'b'; add EDGEDISJOINT as one of
the explicit cases that return ' ' so it is handled as non-driving-side like
ASTAR/BDASTAR/DAGSP/EDWARDMOORE/BELLMANFORD/BDDIJKSTRA/DIJKSTRA, i.e. insert
"case EDGEDISJOINT:" into the same case group that returns ' ' to ensure
consistent behavior.
ℹ️ Review info
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0198519e-4102-48b6-a877-009b3cb174f0
📒 Files selected for processing (24)
NEWS.mddoc/src/release_notes.rstinclude/bellman_ford/bellman_ford.hppinclude/c_common/enums.hinclude/cpp_common/combinations.hppinclude/cpp_common/to_postgres.hppinclude/drivers/bellman_ford/bellman_ford_driver.hinclude/drivers/bellman_ford/bellman_ford_neg_driver.hinclude/drivers/max_flow/edge_disjoint_paths_driver.hinclude/max_flow/maxflow.hpplocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potsrc/bellman_ford/CMakeLists.txtsrc/bellman_ford/bellman_ford.csrc/bellman_ford/bellman_ford_driver.cppsrc/bellman_ford/bellman_ford_neg_driver.cppsrc/cpp_common/combinations.cppsrc/cpp_common/to_postgres.cppsrc/cpp_common/utilities.cppsrc/dijkstra/shortestPath_driver.cppsrc/max_flow/CMakeLists.txtsrc/max_flow/edge_disjoint_paths.csrc/max_flow/edge_disjoint_paths_driver.cppsrc/max_flow/maxflow.cpp
💤 Files with no reviewable changes (7)
- src/max_flow/edge_disjoint_paths_driver.cpp
- include/drivers/max_flow/edge_disjoint_paths_driver.h
- include/drivers/bellman_ford/bellman_ford_driver.h
- src/bellman_ford/bellman_ford_driver.cpp
- src/bellman_ford/bellman_ford_neg_driver.cpp
- include/drivers/bellman_ford/bellman_ford_neg_driver.h
- src/bellman_ford/CMakeLists.txt
Comment on lines
+36
to
+37
| BELLMANFORD, | ||
| EDGEDISJOINT, |
Contributor
There was a problem hiding this comment.
Verify get_name(Which) coverage for newly added enum values.
BELLMANFORD and EDGEDISJOINT are added here, but src/cpp_common/utilities.cpp (the get_name(Which) overload) appears to fall through to "unknown" for these values. Please confirm whether that overload is reachable in these paths and add cases if needed.
#!/bin/bash
set -euo pipefail
echo "=== get_name(Which) implementation ==="
rg -n -A120 -B5 'get_name\s*\(\s*Which\s+which\s*\)' src/cpp_common/utilities.cpp
echo
echo "=== Call sites using get_name(...) ==="
rg -nP '\bget_name\s*\(' src include
echo
echo "=== New enum usage sites ==="
rg -nP '\bBELLMANFORD\b|\bEDGEDISJOINT\b' src includeExpected result: either (a) no reachable path uses get_name(Which) for these enums, or (b) switch cases are added so it never returns "unknown" for them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/c_common/enums.h` around lines 36 - 37, The get_name(Which) overload
in src/cpp_common/utilities.cpp currently returns "unknown" for BELLMANFORD and
EDGEDISJOINT; locate the get_name(Which) function and either (a) confirm no
reachable call sites pass the new enum values by searching call sites that pass
Which, or (b) add explicit switch cases for BELLMANFORD and EDGEDISJOINT in
get_name(Which) so they map to the proper string names (use the enum identifiers
BELLMANFORD and EDGEDISJOINT to find the exact enum and implement the
corresponding case arms), ensuring the function never falls through to "unknown"
for these values.
Comment on lines
+317
to
+326
| auto found = paths.size(); | ||
| for (const auto &e : edges) { | ||
| for (auto &r : paths) { | ||
| if (r.edge == e.id) { | ||
| r.cost = (r.node == e.source) ? e.cost : e.reverse_cost; | ||
| --found; | ||
| } | ||
| } | ||
| if (found == 0) break; | ||
| } |
Contributor
There was a problem hiding this comment.
Add a hard sanity check for unresolved path edges before returning tuples.
At Line 317, the completion counter does not guarantee that every routable (edge >= 0) row was resolved. Any unmatched edge id currently leaves r.cost silently stale, which can return incorrect results.
🛠️ Suggested fix (validate mappings and direction explicitly)
- auto found = paths.size();
- for (const auto &e : edges) {
- for (auto &r : paths) {
- if (r.edge == e.id) {
- r.cost = (r.node == e.source) ? e.cost : e.reverse_cost;
- --found;
- }
- }
- if (found == 0) break;
- }
+ std::map<int64_t, const Edge_t*> edge_by_id;
+ for (const auto &e : edges) {
+ edge_by_id[e.id] = &e;
+ }
+
+ size_t unresolved = 0;
+ for (auto &r : paths) {
+ if (r.edge < 0) continue;
+ auto it = edge_by_id.find(r.edge);
+ if (it == edge_by_id.end()) {
+ ++unresolved;
+ continue;
+ }
+
+ const auto &e = *(it->second);
+ if (r.node == e.source) {
+ r.cost = e.cost;
+ } else if (r.node == e.target) {
+ r.cost = e.reverse_cost;
+ } else {
+ ++unresolved;
+ }
+ }
+ pgassert(unresolved == 0);Based on learnings: In pgRouting's allpairs driver, the "No result generated, report this error" return_count check is used as an internal sanity check to catch implementation bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cpp_common/to_postgres.cpp` around lines 317 - 326, The loop that assigns
costs to path entries (variables paths, edges, found, r.cost, e.id, e.source,
e.cost, e.reverse_cost) can leave routable rows (r.edge >= 0) unresolved; add a
final sanity check after the loop that scans paths for any entry with r.edge >=
0 whose r.cost is still unset/invalid and if any are found abort/throw/log a
fatal error (e.g., mirror pgRouting's "No result generated, report this error")
so unresolved mappings cannot silently return stale costs; ensure the check only
considers routable edges (r.edge >= 0) and includes a clear message identifying
the inconsistency before returning tuples.
Comment on lines
+39
to
+43
| single_execution( | ||
| std::vector<Edge_t> edges, | ||
| int64_t source, | ||
| int64_t target, | ||
| bool directed) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check if the file exists and get basic info
if [ ! -f "src/max_flow/maxflow.cpp" ]; then
echo "File not found: src/max_flow/maxflow.cpp"
exit 1
fi
echo "=== Checking file structure ==="
wc -l src/max_flow/maxflow.cpp
echo -e "\n=== Lines 39-50 (single_execution function) ==="
sed -n '39,50p' src/max_flow/maxflow.cpp
echo -e "\n=== Lines 342-355 (edgeDisjoint function) ==="
sed -n '342,355p' src/max_flow/maxflow.cpp
echo -e "\n=== Searching for single_execution calls ==="
rg -n "single_execution\s*\(" src/max_flow/maxflow.cpp | head -20Repository: pgRouting/pgrouting
Length of output: 1293
🏁 Script executed:
#!/bin/bash
# Check header file for declarations
echo "=== Checking header file ==="
if [ -f "include/max_flow/maxflow.hpp" ]; then
echo "Header file exists. Searching for function declarations:"
echo -e "\n--- edgeDisjoint declaration ---"
rg -A 2 "edgeDisjoint" include/max_flow/maxflow.hpp
echo -e "\n--- single_execution declaration ---"
rg -A 2 "single_execution" include/max_flow/maxflow.hpp
else
echo "Header file not found"
fiRepository: pgRouting/pgrouting
Length of output: 354
🏁 Script executed:
#!/bin/bash
# Check context around the call to single_execution
echo "=== Context around single_execution call in edgeDisjoint ==="
sed -n '342,370p' src/max_flow/maxflow.cppRepository: pgRouting/pgrouting
Length of output: 767
Change edges parameter to const reference in both functions to avoid repeated vector copies in the nested loop.
single_execution and edgeDisjoint both take std::vector<Edge_t> by value. Since edgeDisjoint calls single_execution(edges, ...) inside nested loops over source-target combinations, this creates redundant full-vector copies that degrade performance for large edge sets.
Proposed fix
--- a/src/max_flow/maxflow.cpp
+++ b/src/max_flow/maxflow.cpp
@@ -39,7 +39,7 @@ namespace pgrouting::functions {
std::vector<Path_rt>
single_execution(
- std::vector<Edge_t> edges,
+ const std::vector<Edge_t> &edges,
int64_t source,
int64_t target,
bool directed) {
@@ -342,7 +342,7 @@ single_execution(
std::vector<Path_rt>
edgeDisjoint(
- std::vector<Edge_t> edges,
+ const std::vector<Edge_t> &edges,
const std::map<int64_t, std::set<int64_t>> & combinations,
bool directed) {--- a/include/max_flow/maxflow.hpp
+++ b/include/max_flow/maxflow.hpp
-std::vector<Path_rt> edgeDisjoint(std::vector<Edge_t>, const std::map<int64_t, std::set<int64_t>>&, bool);
+std::vector<Path_rt> edgeDisjoint(const std::vector<Edge_t>&, const std::map<int64_t, std::set<int64_t>>&, bool);🧰 Tools
🪛 Cppcheck (2.19.0)
[information] 41-41: Include file
(missingIncludeSystem)
[information] 42-42: Include file
(missingIncludeSystem)
[information] 43-43: Include file
(missingIncludeSystem)
[performance] 40-40: Function parameter 'edges' should be passed by const reference.
(passedByValue)
[information] 41-41: Include file
(missingIncludeSystem)
[information] 42-42: Include file
(missingIncludeSystem)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/max_flow/maxflow.cpp` around lines 39 - 43, The functions
single_execution and edgeDisjoint currently take std::vector<Edge_t> by value
which causes expensive copies when edgeDisjoint repeatedly calls
single_execution; change both function signatures to take const
std::vector<Edge_t>& edges (and adjust any forward declarations/overloads) so
the vector is passed by const reference. Ensure inside single_execution you do
not mutate edges; if mutation is required, make a local copy there only. Update
all call sites (edgeDisjoint -> single_execution) to match the new signature.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3089
Changes proposed in this pull request:
pgr_edgeDisjointPathsandpgr_bellmanFordfunctions@pgRouting/admins
Summary by CodeRabbit