coloring using one process and driver#3103
Merged
cvvergara merged 16 commits intoMay 20, 2026
Merged
Conversation
Contributor
WalkthroughThis PR centralizes coloring processing into a shared C process and C++ driver, converts per-algorithm drivers/templates into free-function C++ implementations on pgrouting::UndirectedGraph, updates SRF C wrappers to call the shared process, and adjusts tests, docs, locales, build, and tooling. ChangesColoring refactor
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 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)
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: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/update.yml:
- Line 50: The workflow currently sets the GitHub Actions matrix option
fail-fast: true which cancels remaining matrix jobs (including old_pgr entries)
after the first failure; change fail-fast to false (or remove the key) in the
workflow definition so the matrix runs all entries and preserves full signal for
old_pgr jobs—update the fail-fast setting in the job/matrix block where
fail-fast: true is declared.
In `@include/coloring/edgeColoring.hpp`:
- Line 42: The declaration of edgeColoring currently takes
pgrouting::UndirectedGraph by value which causes unnecessary copies; change the
function signature and all matching definitions/overloads to accept a const
reference (const pgrouting::UndirectedGraph& graph) and update any callers to
pass the graph by reference; ensure the header declaration (edgeColoring) and
its implementation use the same const-reference type and rebuild to catch any
mismatched signatures.
In `@include/coloring/sequentialVertexColoring.hpp`:
- Line 40: The declaration of sequentialVertexColoring has inconsistent leading
whitespace; update the function prototype for sequentialVertexColoring(const
pgrouting::UndirectedGraph&) so its indentation matches surrounding declarations
(remove the extra leading spaces before "std::vector<II_t_rt>
sequentialVertexColoring(...)" to align with the file's style).
In `@src/coloring/bipartite.cpp`:
- Line 78: Change the pgr_bipartite function signature to take the graph by
const reference since it is not modified: update the implementation signature
std::vector<II_t_rt> pgr_bipartite(const pgrouting::UndirectedGraph& graph) and
also update the corresponding declaration for pgr_bipartite in the header to
match; ensure any callers still compile (they should) and adjust any non-const
contexts if necessary.
- Around line 58-60: The thrown error message incorrectly references
"boost::edge_coloring" even though this is the bipartite implementation; update
the string passed in the throw (the std::make_pair in bipartite.cpp) to
reference the bipartite implementation (e.g., "INTERNAL: something went wrong
while calling bipartite edge coloring" or similar) so the error accurately names
the failing component while leaving the second element (__PGR_PRETTY_FUNCTION__)
unchanged.
- Around line 78-80: The code currently calls boost::is_bipartite twice (once in
pgr_bipartite and again inside get_bipartition), causing redundant graph
traversal; modify pgr_bipartite and get_bipartition so the bipartiteness result
is computed once and reused—either by changing get_bipartition to accept a
precomputed bool (e.g., add a parameter like bool is_bipartite) or by moving the
partition extraction logic into pgr_bipartite and passing the graph and the
single is_bipartite result to the partition routine; ensure you remove the
internal boost::is_bipartite call from get_bipartition and use the passed flag
to decide whether to return an empty vector or the partition.
In `@src/coloring/coloring_driver.cpp`:
- Around line 103-105: The default error branch concatenates the message and
function name without a separator; update the default case so the err stream
includes a separator before calling get_name(which) (e.g., add a space or ": ")
so the output becomes "Unknown coloring function: EDGECOLORING" when locating
the code around the default branch that writes to err and calls get_name(which).
In `@src/coloring/coloring_process.cpp`:
- Around line 55-56: The using-declaration imports pgr_free but the code calls
pfree directly (see symbols pgr_free and pfree); remove the unused
using-declaration for pgr_free from the top of coloring_process.cpp (keep
to_pg_msg if still used) so there is no dead import, or alternatively replace
calls to pfree with pgr_free if pgr_free is the intended wrapper—ensure
consistency by choosing one symbol (pgr_free or pfree) and updating the using
list and call sites accordingly.
- Around line 57-68: The code asserts edges_sql is non-null via
pgassert(edges_sql) but then uses a defensive ternary edges_sql ? edges_sql : ""
when calling pgrouting::drivers::do_coloring; remove the redundant ternary and
pass edges_sql directly to do_coloring (or conversely remove the pgassert and
keep the ternary if you prefer null-safe behavior) so the checks are
consistent—update the call site in the do_coloring invocation to use edges_sql
and ensure only one null-handling strategy is present.
In `@src/coloring/edgeColoring.c`:
- Around line 3-5: Remove the duplicated header comment "Generated with Template
by:" in src/coloring/edgeColoring.c so it appears only once; edit the
top-of-file comment block (the header around the existing "Generated with
Template by:" lines) to eliminate the redundant line and preserve any
surrounding header text or metadata.
In `@src/coloring/edgeColoring.cpp`:
- Around line 80-85: Fix the comment typo in edgeColoring.cpp: change
"outsatnding" to "outstanding" in the block above the assignment that sets
results[i].d2.value (the comment that references boost and color_map/get). Leave
the code statement (results[i].d2.value = ...) unchanged.
- Around line 48-49: The edgeColoring function currently takes the graph
parameter by value (edgeColoring(pgrouting::UndirectedGraph g)), causing an
unnecessary copy; change the signature to accept a const reference
(edgeColoring(const pgrouting::UndirectedGraph& g)) and update any callers if
needed, ensuring you only read from g inside edgeColoring (or explicitly copy
where mutation is required) and keep references to member functions or iterators
compatible with a const graph.
In `@src/coloring/sequentialVertexColoring.cpp`:
- Around line 104-114: The catch-all handler after calling
boost::sequential_vertex_coloring incorrectly mentions "boost::edge_coloring" in
the thrown error; update the error message text in the throw inside the
catch(...) to reference "boost::sequential_vertex_coloring" (and ensure the
thrown pair still contains the descriptive message plus
__PGR_PRETTY_FUNCTION__), and remove any stray punctuation (extra semicolon) if
present so the exception text correctly identifies the failing function.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef849f12-4b0b-4875-8445-cc31c475d5e9
📒 Files selected for processing (28)
.github/workflows/update.ymlNEWS.mddoc/coloring/pgr_edgeColoring.rstdoc/src/release_notes.rstdocqueries/coloring/edgeColoring.resultinclude/c_common/enums.hinclude/coloring/bipartite.hppinclude/coloring/bipartite_driver.hppinclude/coloring/edgeColoring.hppinclude/coloring/sequentialVertexColoring.hppinclude/drivers/coloring_driver.hppinclude/process/coloring_process.hlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potpgtap/coloring/edgeColoring/edge_cases.pgsrc/coloring/CMakeLists.txtsrc/coloring/bipartite.csrc/coloring/bipartite.cppsrc/coloring/bipartite_driver.cppsrc/coloring/coloring_driver.cppsrc/coloring/coloring_process.cppsrc/coloring/edgeColoring.csrc/coloring/edgeColoring.cppsrc/coloring/edgeColoring_driver.cppsrc/coloring/sequentialVertexColoring.csrc/coloring/sequentialVertexColoring.cppsrc/coloring/sequentialVertexColoring_driver.cpptools/scripts/code_checker.sh
💤 Files with no reviewable changes (4)
- src/coloring/sequentialVertexColoring_driver.cpp
- src/coloring/bipartite_driver.cpp
- src/coloring/edgeColoring_driver.cpp
- include/coloring/bipartite_driver.hpp
robe2
approved these changes
Mar 31, 2026
Member
robe2
left a comment
robe2
left a comment
There was a problem hiding this comment.
Make sure to review coderabbitai major issues
cvvergara
changed the title
cvvergara
force-pushed
the
coloring-using-one-process_and-driver
branch
from
ad92dc2 to
b561cd0
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@doc/src/release_notes.rst`:
- Line 52: Fix the typo in the release note entry "* :issue:`3100`: Coloring:
create an use a process & driver" by changing "create an use" to "create and
use" so the line reads "* :issue:`3100`: Coloring: create and use a process &
driver" (update the string in doc/src/release_notes.rst where the issue 3100
entry appears).
In `@locale/en/LC_MESSAGES/pgrouting_doc_strings.po`:
- Around line 3974-3977: Update the msgid string that reads "`#3100
<https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring: create an use
a process & driver" by changing "an use" to "and use"; specifically edit the
msgid in the PO entry (the quoted string starting with "`#3100 ... Coloring:
create an use a process & driver") to read "... Coloring: create and use a
process & driver" and, if applicable, adjust the corresponding msgstr to the
corrected English text or provide the proper translation.
In `@locale/pot/pgrouting_doc_strings.pot`:
- Line 3559: Update the message string with the typo fix: locate the msgid text
"`#3100 <https://github.com/pgRouting/pgrouting/issues/3100>`__: Coloring:
create an use a process & driver" and change "create an use" to "create and use"
so the entry reads "Coloring: create and use a process & driver".
In `@NEWS.md`:
- Line 27: Update the release note text for PR `#3100` that currently reads
"create an use a process & driver" to the corrected phrase "create and use a
process & driver" by replacing "an" with "and" in that NEWS.md entry.
In `@pgtap/coloring/edgeColoring/edge_cases.pg`:
- Around line 110-116: The VALUES fixtures feeding pgr_edgeColoring are
unordered and can produce flaky results; update each PREPARE statement (e.g.,
PREPARE q8, q9, q10, q11, q12, q15) that selects FROM (VALUES ...) AS t(...) to
wrap the VALUES subquery with an explicit ORDER BY id ASC so the edge rows are
deterministically ordered before calling pgr_edgeColoring; ensure every
VALUES-based query in this file using ids (the same pattern around
q8/q9/q10/q11/q12/q15) gets the ORDER BY id ASC applied.
- Around line 258-259: The q15 test only asserts count(DISTINCT color)=1 but not
that both disconnected edges are returned; update the single TAP assertion for
q15 to validate both output cardinality and color uniqueness in one check
against pgr_edgeColoring('q15') — e.g., replace the current distinct-color-only
assertion with a single assertion that ensures the result set has two rows (both
edges returned) and that count(DISTINCT color)=1, referencing
pgr_edgeColoring('q15') and keeping it as one TAP is() assertion.
In `@src/coloring/bipartite.cpp`:
- Around line 46-48: Replace the wrapper call graph.num_vertices() with the
direct BGL function boost::num_vertices(graph.graph) when sizing the partition
vector and creating partition_map; update the allocation of
std::vector<boost::default_color_type> partition(...) to use
boost::num_vertices(graph.graph) and ensure make_iterator_property_map still
uses partition.begin() with boost::get(boost::vertex_index, graph.graph) so the
code references the underlying Boost graph directly (symbols: partition,
partition_map, graph.num_vertices(), boost::num_vertices(graph.graph)).
In `@src/coloring/coloring_driver.cpp`:
- Around line 52-61: At the top of do_coloring initialize the output parameters
to safe defaults so callers never see stale values: set return_tuples to null
(no allocated pointer) and return_count to 0 before any branching/early returns,
and ensure any early-return paths leave those defaults intact; update all
branches that currently return early (including the error/notice paths
referenced around the shown ranges) to rely on these initialized values rather
than assuming they were set later.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 227e211a-8824-41d5-90fa-da7a13a67b9d
📒 Files selected for processing (28)
.github/workflows/update.ymlNEWS.mddoc/coloring/pgr_edgeColoring.rstdoc/src/release_notes.rstdocqueries/coloring/edgeColoring.resultinclude/c_common/enums.hinclude/coloring/bipartite.hppinclude/coloring/bipartite_driver.hppinclude/coloring/edgeColoring.hppinclude/coloring/sequentialVertexColoring.hppinclude/drivers/coloring_driver.hppinclude/process/coloring_process.hlocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potpgtap/coloring/edgeColoring/edge_cases.pgsrc/coloring/CMakeLists.txtsrc/coloring/bipartite.csrc/coloring/bipartite.cppsrc/coloring/bipartite_driver.cppsrc/coloring/coloring_driver.cppsrc/coloring/coloring_process.cppsrc/coloring/edgeColoring.csrc/coloring/edgeColoring.cppsrc/coloring/edgeColoring_driver.cppsrc/coloring/sequentialVertexColoring.csrc/coloring/sequentialVertexColoring.cppsrc/coloring/sequentialVertexColoring_driver.cpptools/scripts/code_checker.sh
💤 Files with no reviewable changes (4)
- src/coloring/sequentialVertexColoring_driver.cpp
- include/coloring/bipartite_driver.hpp
- src/coloring/bipartite_driver.cpp
- src/coloring/edgeColoring_driver.cpp
Comment on lines
+110
to
+116
| PREPARE q8 AS | ||
| SELECT id, source, target, cost, reverse_cost | ||
| FROM three_vertices_table; | ||
| FROM (VALUES | ||
| (1, 3, 6, 20, 15), | ||
| (2, 3, 8, 10, -10), | ||
| (3, 6, 8, -1, 12)) | ||
| AS t(id, source, target, cost, reverse_cost); |
Contributor
There was a problem hiding this comment.
Add explicit ORDER BY for VALUES fixtures used by an order-sensitive algorithm.
Line 112 (and the same pattern in Lines 132, 149, 171, 191, 253) feeds pgr_edgeColoring from unordered VALUES scans. Since this test file already proves output changes with edge order (q6/q7), missing ORDER BY can make expected color-count assertions flaky across planners/versions.
Proposed fix
PREPARE q8 AS
SELECT id, source, target, cost, reverse_cost
FROM (VALUES
(1, 3, 6, 20, 15),
(2, 3, 8, 10, -10),
(3, 6, 8, -1, 12))
-AS t(id, source, target, cost, reverse_cost);
+AS t(id, source, target, cost, reverse_cost)
+ORDER BY id ASC;Apply the same ORDER BY id ASC pattern to q9, q10, q11, q12, and q15.
Also applies to: 130-138, 148-152, 171-174, 191-199, 253-256
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pgtap/coloring/edgeColoring/edge_cases.pg` around lines 110 - 116, The VALUES
fixtures feeding pgr_edgeColoring are unordered and can produce flaky results;
update each PREPARE statement (e.g., PREPARE q8, q9, q10, q11, q12, q15) that
selects FROM (VALUES ...) AS t(...) to wrap the VALUES subquery with an explicit
ORDER BY id ASC so the edge rows are deterministically ordered before calling
pgr_edgeColoring; ensure every VALUES-based query in this file using ids (the
same pattern around q8/q9/q10/q11/q12/q15) gets the ORDER BY id ASC applied.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/coloring/edgeColoring.cpp`:
- Around line 48-49: The function signature currently takes the graph by value
(edgeColoring(const pgrouting::UndirectedGraph g)), causing an unnecessary copy
and mismatching the header; change it to take a const reference
(edgeColoring(const pgrouting::UndirectedGraph &g)) in the implementation and
update the declaration in include/coloring/edgeColoring.hpp (line with
edgeColoring) to the identical const reference form so both compile-time
signatures match and avoid copying the graph.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e38fdc63-ae2a-40f1-aba4-45ab7ec0e3d0
📒 Files selected for processing (7)
include/coloring/bipartite.hppsrc/coloring/bipartite.cppsrc/coloring/coloring_driver.cppsrc/coloring/coloring_process.cppsrc/coloring/edgeColoring.csrc/coloring/edgeColoring.cppsrc/coloring/sequentialVertexColoring.cpp
💤 Files with no reviewable changes (1)
- src/coloring/edgeColoring.c
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/coloring/bipartite.cpp (1)
58-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winExtra semicolon at end of statement.
There's a double semicolon at the end of line 60.
Proposed fix
throw std::make_pair( std::string("INTERNAL: something went wrong while calling boost::bipartite"), - std::string(__PGR_PRETTY_FUNCTION__));; + std::string(__PGR_PRETTY_FUNCTION__));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coloring/bipartite.cpp` around lines 58 - 60, The throw statement that constructs and throws a std::pair using std::make_pair(..., std::string(__PGR_PRETTY_FUNCTION__)) has an extra semicolon at the end; remove the redundant semicolon so the line ends with a single semicolon. Locate the throw that uses std::make_pair and __PGR_PRETTY_FUNCTION__ in bipartite.cpp and change "));;" to ");" to fix the syntax error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/coloring/bipartite.cpp`:
- Around line 58-60: The throw statement that constructs and throws a std::pair
using std::make_pair(..., std::string(__PGR_PRETTY_FUNCTION__)) has an extra
semicolon at the end; remove the redundant semicolon so the line ends with a
single semicolon. Locate the throw that uses std::make_pair and
__PGR_PRETTY_FUNCTION__ in bipartite.cpp and change "));;" to ");" to fix the
syntax error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 535adfbc-d3e7-414c-a1a1-455861eab99f
📒 Files selected for processing (9)
NEWS.mddoc/src/release_notes.rstinclude/coloring/edgeColoring.hpplocale/en/LC_MESSAGES/pgrouting_doc_strings.polocale/pot/pgrouting_doc_strings.potpgtap/coloring/edgeColoring/edge_cases.pgsrc/coloring/bipartite.cppsrc/coloring/coloring_driver.cppsrc/coloring/edgeColoring.cpp
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
2 participants
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 #3100 #3101
Changes proposed in this pull request:
@pgRouting/admins
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Refactor
Chores