Conversation
Introduces three new shortest path algorithms: - Bidirectional BFS for unweighted graphs (O(b^(d/2)) time/space). - Dial's algorithm for small integer weighted graphs (O(V + E + V*C) time). - IDA* for memory-efficient heuristic search (O(b^d) time, O(d) space). Includes new test files and examples for each algorithm. Updates CMakeLists.txt and READMEs to reflect the new features and examples.
Contributor
WalkthroughAdds four new shortest-path algorithm headers (Bidirectional BFS, Dial, 0-1 BFS, IDA*), example programs and CMake entries, extensive GoogleTest suites for each algorithm, small fixes to tpl_interval_tree.H, a test-run helper normalization in Tests/run_one_test.rb, and minor README updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bidirectional_BFS
participant Forward_Frontier
participant Backward_Frontier
participant Meeting_Point
Client->>Bidirectional_BFS: find_path(g, start, end)
Bidirectional_BFS->>Forward_Frontier: init(start)
Bidirectional_BFS->>Backward_Frontier: init(end)
loop until meeting
Bidirectional_BFS->>Forward_Frontier: pop & expand neighbors
Forward_Frontier-->>Bidirectional_BFS: discovered nodes
Bidirectional_BFS->>Meeting_Point: check if backward-visited
alt not met
Bidirectional_BFS->>Backward_Frontier: pop & expand neighbors
Backward_Frontier-->>Bidirectional_BFS: discovered nodes
Bidirectional_BFS->>Meeting_Point: check if forward-visited
end
end
Bidirectional_BFS->>Bidirectional_BFS: build_path(start, meeting, end)
Bidirectional_BFS-->>Client: return shortest path
sequenceDiagram
participant Client
participant Dial_Min_Paths
participant Bucket_Array
participant Node_Info
Client->>Dial_Min_Paths: paint_min_paths_tree(g, start)
Dial_Min_Paths->>Dial_Min_Paths: validate non-negative integer weights
Dial_Min_Paths->>Bucket_Array: init buckets based on max weight
loop process buckets
Dial_Min_Paths->>Bucket_Array: get bucket[current_dist]
Bucket_Array-->>Dial_Min_Paths: nodes list
loop each node
Dial_Min_Paths->>Node_Info: read distance & relax outgoing arcs
Dial_Min_Paths->>Bucket_Array: insert neighbor into bucket[new_dist]
Node_Info-->>Dial_Min_Paths: update parent/dist
end
end
Dial_Min_Paths-->>Client: painted shortest-path tree
sequenceDiagram
participant Client
participant IDA_Star
participant Search
participant Path_Accumulator
Client->>IDA_Star: find_path(g, start, end)
IDA_Star->>IDA_Star: threshold = h(start,end)
loop until FOUND or Inf
IDA_Star->>Search: search(start, 0, threshold)
Search->>Search: compute f = g + h(curr,end)
alt f > threshold
Search-->>IDA_Star: return min_exceeding
else if curr == end
Search-->>IDA_Star: FOUND (path in Path_Accumulator)
else
Search->>Path_Accumulator: push arc
Search->>Search: recurse on neighbor
Search->>Path_Accumulator: pop arc
end
IDA_Star->>IDA_Star: threshold = next_min_exceeding
end
IDA_Star-->>Client: return path or Inf
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd0dcfb462
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds several shortest-path/search algorithms to the Aleph-w graph toolkit (0-1 BFS, Dial’s algorithm, bidirectional BFS, IDA*) along with new examples and GoogleTest-based coverage, and updates build/docs to include them.
Changes:
- Added new graph algorithm headers:
Zero_One_BFS.H,Dial.H,Bidirectional_BFS.H,IDA_Star.H. - Added comprehensive GTest suites and registered them in
Tests/CMakeLists.txt. - Added runnable examples and registered them in
Examples/CMakeLists.txt, plus README catalog updates.
Reviewed changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
Zero_One_BFS.H |
New 0-1 BFS shortest-path implementation with “paint tree” + extract path APIs. |
Dial.H |
New Dial’s (bucket) shortest-path implementation for non-negative integer weights. |
Bidirectional_BFS.H |
New bidirectional BFS implementation for unweighted shortest paths. |
IDA_Star.H |
New IDA* implementation + Chebyshev heuristic helper. |
Tests/test_zero_one_bfs.cc |
New test coverage for 0-1 BFS, including edge cases and Dijkstra cross-check. |
Tests/test_dial.cc |
New test coverage for Dial, including edge cases and Dijkstra cross-check. |
Tests/test_bidirectional_bfs.cc |
New test coverage for bidirectional BFS, including directed-graph iterator configuration. |
Tests/test_ida_star.cc |
New test coverage for IDA* including cross-validation vs A* / Dijkstra. |
Tests/CMakeLists.txt |
Registers the new test executables. |
Examples/zero_one_bfs_example.cc |
New usage example for 0-1 BFS. |
Examples/dial_example.cc |
New usage example for Dial. |
Examples/bidirectional_bfs_example.cc |
New usage example for bidirectional BFS. |
Examples/ida_star_example.cc |
New usage example for IDA*. |
Examples/CMakeLists.txt |
Registers the new example executables. |
README.md |
Updates algorithm/catalog listing to mention the new additions. |
![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: 17
🧹 Nitpick comments (4)
Examples/zero_one_bfs_example.cc (1)
40-84: Example clarity: Node_Info should bechar(notint) since you store/print letters.Must fix:
- None.
Should fix:
- Change
using Node_Info = int;tousing Node_Info = char;and drop the casts when printing.Nice to have:
std::fixedisn’t doing anything for integer distances; you can remove it to keep output formatting intent crisp.Based on learnings: In the Aleph-w codebase, maintain compatibility with older compilers that do not provide C++20 std::format. Do not flag or require replacing stream-based output (std::cout << ...), as long as the build targets do not guarantee std::format support.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/zero_one_bfs_example.cc` around lines 40 - 84, Change the Node_Info alias from int to char (replace "using Node_Info = int;" with "using Node_Info = char;") so node info stores letters; update the print loop to drop the explicit cast "(char)node->get_info()" and print node->get_info() directly, and optionally remove std::fixed since distances are integers to keep formatting intent clear; keep all uses of Zero_One_BFS, paint_min_paths_tree, and get_distance unchanged.Tests/test_dial.cc (1)
168-178: Should fix: add a negative-weight validation test.The suite covers null-node validation, but it should also assert behavior for invalid negative edge weights to lock precondition handling.
Based on learnings: "Validate input preconditions (e.g., non-negative weights for Dijkstra)" and "Tests must verify algorithmic correctness, not just absence of crashes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/test_dial.cc` around lines 168 - 178, Add a test that verifies Dial_Min_Paths rejects negative edge weights: construct a graph GT (as in the existing test), insert two nodes, add an edge between them with a negative weight, then call Dial_Min_Paths<GT>::operator()(g, source, target, path) and assert it throws std::domain_error; reference the same symbols used in the file (GT, insert_node, Dial_Min_Paths<GT> dial, Path<GT> path, and dial(g, ...)) so the new test mirrors NullNodeValidation but checks negative-weight validation instead of null nodes.Tests/test_zero_one_bfs.cc (1)
224-233: Nice to have: makeEmptyGraphValidationexercise algorithm behavior.This test currently validates only
g.get_num_nodes() == 0and never callsZero_One_BFS, so it does not verify the empty-graph contract of the API.Based on learnings: "Tests must verify algorithmic correctness, not just absence of crashes."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/test_zero_one_bfs.cc` around lines 224 - 233, The test EmptyGraphValidation currently only checks g.get_num_nodes() == 0 and never exercises the algorithm; update it to actually invoke the Zero_One_BFS algorithm on the empty graph (use the same public entry point used elsewhere in tests for Zero_One_BFS<GT>, e.g., the run/compute/execute method you use to produce a Path<GT>) and assert the expected empty-graph contract: either expect a well-defined failure (use EXPECT_THROW if the API throws for empty graphs) or expect a successful call that yields an empty/zero-length Path (check Path<GT>::empty() or size()==0 or the API’s equivalent). Keep references to Zero_One_BFS<GT>, Path<GT>, g and the TEST name EmptyGraphValidation so the change is localized to this test.README.md (1)
212-213: Documentation update looks good.
- Must fix: None.
- Should fix: None.
- Nice to have: Consider expanding
ReservoirtoReservoir Samplingfor naming consistency with the later dedicated section/title.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 212 - 213, Update the listing entry currently labeled "Reservoir" to "Reservoir Sampling" so it matches the dedicated section/title later; locate the string "Reservoir" in the README (the list line containing "HyperLogLog/MinHash/SimHash/Reservoir") and replace it with "Reservoir Sampling" to maintain naming consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Bidirectional_BFS.H`:
- Around line 141-158: The init/cleanup pattern is not exception-safe:
init(const GT& g) allocates BFS_Info via new and stores raw pointers in
NODE_COOKIE, but if anything throws afterward cleanup(g) may never run and nodes
will leak or dangle; fix by making per-call ownership RAII: replace raw
new/delete usage by storing std::unique_ptr<BFS_Info> (or an internal container
of unique_ptrs) and set NODE_COOKIE to the unique_ptr.get() (or store nullptr
when cleared), or introduce a small local guard class (e.g., BiBFS_Init_Guard)
that runs cleanup(g) in its destructor and is created immediately after init(g)
so cleanup always runs on scope exit; ensure cleanup still nulls NODE_COOKIE and
releases ownership (unique_ptr reset or delete) and update references to
BIBFS_INFO(p) accordingly to avoid dangling pointers.
- Around line 41-49: The Bidirectional_BFS implementation currently expands
nodes node-by-node and may return a non-shortest path; update the algorithm in
the Bidirectional_BFS function to perform layer-by-layer BFS expansions (expand
entire frontier level at once) and only declare success when an intersection is
detected at the minimal depth-sum (or alternatively maintain and check distance
maps dist_forward and dist_backward to prove no shorter meeting point exists);
also change the expansion strategy to always expand the smaller frontier
(implement the "expand smaller frontier" behavior described in the header), exit
early if either frontier queue becomes empty (no path), factor the duplicated
logic into a helper like expand_frontier(direction, frontier_queue, visited_set,
dist_map, next_frontier) to avoid code duplication, and verify visited set and
distance updates are consistent and symmetric between forward/backward
traversals so meeting detection is correct.
- Around line 160-213: build_path currently reconstructs the path by
re-searching arcs with search_arc and silently skips when arc == nullptr, which
can desync the Path; modify the algorithm to record and use the parent Arc* for
each node during the forward/backward BFS expansion (e.g., store per-node parent
arc alongside BIBFS_FWD_PARENT and BIBFS_BCK_PARENT) and then in build_path use
those stored parent Arc*s to append to Path<GT> instead of calling search_arc;
if you must keep search_arc, validate that the found arc satisfies the same SA
filter used during expansion and fail (return or throw) rather than silently
skipping when no valid arc is found so the reconstructed Path cannot become
inconsistent.
In `@Dial.H`:
- Around line 158-191: The init() function allocates Node_Info objects stored
via NODE_COOKIE but if run_dial() or any subsequent throwing code runs before
cleanup these allocations leak and NODE_COOKIE remains corrupted; fix by
ensuring uninit_discard() is invoked on any exception after init() (e.g. wrap
the post-init work including run_dial() in a try/catch or use a small RAII guard
that calls uninit_discard() on destruction unless committed), restore ptr_g and
painted/s state consistently on failure, and ensure commit happens only after
successful completion so uninit_paint()/uninit_discard() semantics match (refer
to functions init(), run_dial(), uninit_discard(), uninit_paint(), NODE_COOKIE,
Node_Info, and ptr_g when implementing the guard).
- Around line 245-252: Compute max_dist with unsigned/checked arithmetic to
avoid signed overflow (use size_t or builtin checked mul) when evaluating
(g.get_num_nodes() - 1) * max_w, then validate that the resulting num_buckets
(derived from Distance_Type -> size_t) is within a sane upper bound before
constructing std::vector<DynList<Node*>> buckets; if it exceeds the limit,
return an error or switch to a heap-based Dijkstra fallback. Also add an
explicit overflow check before computing new_dist = curr_dist + w (e.g., ensure
curr_dist <= max_allowed_dist - w) and handle the overflow case safely instead
of indexing buckets[new_dist]. Reference symbols: g.get_num_nodes(), max_w,
Distance_Type, num_buckets, buckets, curr_dist, w, and new_dist.
- Around line 245-252: Dial_Min_Paths can leave stale spanning-tree marks when a
node's parent arc is updated (or when parallel arcs exist); detect when
DIAL_PARENT(tgt) changes and before setting the new Spanning_Tree mark, clear
any previously marked arc(s) for that child so multiple arcs are not left
painted (do not assume the node is already in a painted "tree"). Implement this
by storing/remembering the currently chosen parent arc id for each node (or
deterministically clearing any marked incoming arcs for tgt) in the
Dial_Min_Paths logic where parent updates occur, then mark only the new chosen
arc; apply the same fix to the other similar blocks referenced (around the other
ranges noted) and add/update a unit test with multi-edges between the same
endpoints to ensure get_distance()/get_min_path() resolves the correct arc.
- Around line 250-252: The buckets declaration currently uses
std::vector<DynList<Node *>> buckets(num_buckets) which violates the “use Aleph
containers” rule; replace std::vector with the appropriate Aleph container
(e.g., Aleph::Array, Aleph::DynArray, or Aleph::FixedArray) that supports
construction of num_buckets DynList<Node*> entries and update any code that
accesses buckets accordingly (look for the symbol buckets and the surrounding
allocation logic using num_buckets in Dial.H); if no Aleph container can
represent a run-time-sized array of DynList<Node*> explain this constraint in
the Dial.H API/docs (not just an inline comment) and add a brief justification
comment referencing the lack of an Aleph equivalent.
In `@Examples/bidirectional_bfs_example.cc`:
- Around line 8-80: Replace the std::vector queries with an Aleph container or
remove the container and call queries directly (locate the variable named
queries in main which currently uses vector<pair<GT::Node *, GT::Node *>>), and
when inserting edges call g.insert_arc(src, tgt, (Arc_Info)0) (or otherwise pass
an explicit Arc_Info value) instead of relying on a two-arg overload (update all
g.insert_arc(...) calls); also add missing headers <string> and <utility> to the
top of the file so Node_Info and pair usage have explicit includes.
In `@Examples/ida_star_example.cc`:
- Around line 46-47: Replace standard containers with Aleph equivalents: change
the multi-dimensional std::vector declaration for obstacles
(vector<vector<bool>> obstacles) to use Aleph::DynList<...> for the inner and
outer lists, and similarly replace any other std::vector or std::map usages
referenced around lines 75-82 with Aleph::DynList<T> and Aleph::DynMapTree<K,V>
respectively; update any calls that rely on std::vector/.map APIs to the Aleph
container APIs, and add the necessary Aleph include(s) and namespace
qualification (or using) so symbols like DynList and DynMapTree resolve.
In `@IDA_Star.H`:
- Around line 185-190: Replace the sentinel FOUND usage (static constexpr
Distance_Type FOUND = Distance_Type(-1)) by changing search() to return a small
result struct (e.g., struct SearchResult { bool found; Distance_Type value; })
where value is total_cost when found or next_threshold when not; update callers
to check result.found instead of comparing to FOUND. Add runtime validation
ah_domain_error_if(w < 0) where arc weights are read/used (e.g., in
relax/neighbor loops and any Dijkstra entry points) to enforce non-negative
weights. Ensure Find_Path bits are cleared on every exit path of
search()/related routines (success and all failure/early-return paths) so
visited state is never left dirty. Finally add a static_assert that
Distance_Type is arithmetic (or a minimal requires/concept) to validate the
Distance/Heuristic contract at compile time.
In `@README.md`:
- Around line 258-259: The algorithm name "Bidirectional" in the README list
should be changed to "Bidirectional BFS" to match the PR feature name and avoid
ambiguity; update the string "Bidirectional" in the algorithms list (the line
containing "0-1 BFS / Dial / Bidirectional / IDA*") to "Bidirectional BFS" and
ensure the same exact name is used in any related headers, examples, and tests
for consistency and grepability.
In `@Tests/test_bidirectional_bfs.cc`:
- Around line 668-672: Remove the custom test entry point by deleting the main()
function defined in Tests/test_bidirectional_bfs.cc (the block that calls
::testing::InitGoogleTest(&argc, argv) and return RUN_ALL_TESTS()), since
GTest::gtest_main already provides main; ensure no other custom main remains in
this file to avoid duplicate symbol linker errors.
In `@Tests/test_dial.cc`:
- Around line 623-627: Remove the user-defined test entry point in test_dial.cc:
delete the main function that calls ::testing::InitGoogleTest and RUN_ALL_TESTS
so the target uses the provided GTest::gtest_main implementation; locate the
function named main (which calls ::testing::InitGoogleTest(&argc, argv) and
return RUN_ALL_TESTS()) and remove it from the file to avoid multiple-definition
linker errors.
In `@Tests/test_ida_star.cc`:
- Around line 607-611: Remove the duplicate test entry point by deleting the
custom main() function currently defined (int main(int argc, char **argv) {
::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); }) so the test
binary uses the GTest::gtest_main provided main(), or alternatively stop linking
GTest::gtest_main and keep the custom main(); ensure only one main symbol
remains (reference: the main() function and GTest::gtest_main).
In `@Tests/test_zero_one_bfs.cc`:
- Around line 730-734: Remove the local test entrypoint definitions (the int
main(...) functions) from the listed test files—Tests/test_zero_one_bfs.cc,
Tests/test_ida_star.cc, Tests/test_dial.cc, and
Tests/test_bidirectional_bfs.cc—so they don't collide with GTest::gtest_main;
specifically delete the entire main function block (the
::testing::InitGoogleTest(...) and return RUN_ALL_TESTS() lines) in each file
and leave the existing TEST/TEST_F cases and includes intact so gtest_main
provided by CMake can handle test registration and execution.
In `@Zero_One_BFS.H`:
- Around line 158-191: init() currently allocates Node_Info into NODE_COOKIE for
every node but if run_bfs() throws (e.g., on invalid weight) those allocations
leak and ptr_g/painted/s remain inconsistent; modify the control flow so that
after init() sets up NODE_COOKIE and ptr_g you immediately install a small
RAII/guard object (or try/catch) that on failure calls uninit_discard() and
resets ptr_g/painted/s, and have run_bfs() invoked inside that guarded scope so
any exception triggers rollback; specifically, add the guard around calls to
init(), run_bfs(), and the existing uninit_paint()/uninit_discard() paths so
functions like init(), run_bfs(), uninit_discard(), uninit_paint(), NODE_COOKIE,
ZOB_INFO, ptr_g, painted and s are used to restore a clean state on error.
- Around line 193-236: The BFS currently marks
ARC_BITS(arc).set_bit(Aleph::Spanning_Tree,true) when relaxing a node but never
clears any old spanning-tree arc(s) when ZOB_PARENT(tgt) is updated, causing
stale marks for multi-edges; modify run_bfs so that when you update
ZOB_PARENT(tgt) you first clear the Spanning_Tree bit on the previously chosen
arc(s) for that tgt (e.g., by storing the previously chosen arc id or scanning
incident arcs to clear ARC_BITS(prev_arc).clear_bit(Aleph::Spanning_Tree) before
setting the new one), or better, track and store the chosen parent arc alongside
ZOB_PARENT so you can deterministically clear the old parent arc before marking
the new arc; ensure the change updates any data structures that reference the
parent arc and add tests for parallel edges with differing 0/1 weights to verify
the painted structure is a tree.
---
Nitpick comments:
In `@Examples/zero_one_bfs_example.cc`:
- Around line 40-84: Change the Node_Info alias from int to char (replace "using
Node_Info = int;" with "using Node_Info = char;") so node info stores letters;
update the print loop to drop the explicit cast "(char)node->get_info()" and
print node->get_info() directly, and optionally remove std::fixed since
distances are integers to keep formatting intent clear; keep all uses of
Zero_One_BFS, paint_min_paths_tree, and get_distance unchanged.
In `@README.md`:
- Around line 212-213: Update the listing entry currently labeled "Reservoir" to
"Reservoir Sampling" so it matches the dedicated section/title later; locate the
string "Reservoir" in the README (the list line containing
"HyperLogLog/MinHash/SimHash/Reservoir") and replace it with "Reservoir
Sampling" to maintain naming consistency.
In `@Tests/test_dial.cc`:
- Around line 168-178: Add a test that verifies Dial_Min_Paths rejects negative
edge weights: construct a graph GT (as in the existing test), insert two nodes,
add an edge between them with a negative weight, then call
Dial_Min_Paths<GT>::operator()(g, source, target, path) and assert it throws
std::domain_error; reference the same symbols used in the file (GT, insert_node,
Dial_Min_Paths<GT> dial, Path<GT> path, and dial(g, ...)) so the new test
mirrors NullNodeValidation but checks negative-weight validation instead of null
nodes.
In `@Tests/test_zero_one_bfs.cc`:
- Around line 224-233: The test EmptyGraphValidation currently only checks
g.get_num_nodes() == 0 and never exercises the algorithm; update it to actually
invoke the Zero_One_BFS algorithm on the empty graph (use the same public entry
point used elsewhere in tests for Zero_One_BFS<GT>, e.g., the
run/compute/execute method you use to produce a Path<GT>) and assert the
expected empty-graph contract: either expect a well-defined failure (use
EXPECT_THROW if the API throws for empty graphs) or expect a successful call
that yields an empty/zero-length Path (check Path<GT>::empty() or size()==0 or
the API’s equivalent). Keep references to Zero_One_BFS<GT>, Path<GT>, g and the
TEST name EmptyGraphValidation so the change is localized to this test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa2a8c0d-3a16-41b7-9403-bf6a8a80f1d3
📒 Files selected for processing (16)
Bidirectional_BFS.HDial.HExamples/CMakeLists.txtExamples/bidirectional_bfs_example.ccExamples/dial_example.ccExamples/ida_star_example.ccExamples/zero_one_bfs_example.ccIDA_Star.HREADME.es.mdREADME.mdTests/CMakeLists.txtTests/test_bidirectional_bfs.ccTests/test_dial.ccTests/test_ida_star.ccTests/test_zero_one_bfs.ccZero_One_BFS.H
Introduces three new shortest path algorithms: - Bidirectional BFS for unweighted graphs (O(b^(d/2)) time/space). - Dial's algorithm for small integer weighted graphs (O(V + E + V*C) time). - IDA* for memory-efficient heuristic search (O(b^d) time, O(d) space). Includes new test files and examples for each algorithm. Updates CMakeLists.txt and READMEs to reflect the new features and examples.
lrleon
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
Zero_One_BFS.H (1)
361-394: Consider caching distance in painted state instead of recomputing.The
get_distancemethod walks the entire path from node to start, which is O(path_length). Since distances were already computed during BFS (stored inNode_Info::dist), consider preserving this value duringuninit_paint()instead of recomputing. This would makeget_distanceO(1).Currently
uninit_paint()replacesNODE_COOKIEwith just the parent pointer, discarding the computed distance.💡 Sketch: Preserve distance in a combined struct
struct Painted_Info { Distance_Type dist; Node * parent; }; void uninit_paint() { for (typename GT::Node_Iterator it(*ptr_g); it.has_curr(); it.next()) { auto p = it.get_curr(); auto info = ZOB_INFO(p); - auto parent = info->parent; + auto painted = new Painted_Info{info->dist, info->parent}; delete info; - NODE_COOKIE(p) = parent; + NODE_COOKIE(p) = painted; } }Then
get_distancebecomes:return static_cast<Painted_Info*>(NODE_COOKIE(node))->dist;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Zero_One_BFS.H` around lines 361 - 394, The get_distance method recomputes path length by walking parents because uninit_paint() overwrites NODE_COOKIE with only the parent pointer; fix by preserving the precomputed distance in NODE_COOKIE during painting/uninit_paint: introduce a small struct (e.g., Painted_Info) that holds both parent pointer and the Node_Info::dist value, update the painting code and uninit_paint to store that struct in NODE_COOKIE, and change get_distance to return static_cast<Painted_Info*>(NODE_COOKIE(node))->dist for O(1) access while keeping existing parent traversal logic intact.Bidirectional_BFS.H (1)
112-116: Default iterator parameters may be incorrect for directed graphs.The default
Fwd_ItorandBck_Itorboth useNode_Arc_Iterator, which iterates over all incident arcs. For directed graphs, the backward search fromendneedsIn_Iteratorto traverse incoming arcs, not outgoing ones. The documentation at lines 60-65 correctly advises users to override these defaults for digraphs, but the defaults themselves could lead to incorrect results if users forget.Consider adding a compile-time or runtime diagnostic when using the defaults with a digraph type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dial.H`:
- Around line 99-101: Update the documentation comment in Dial.H to tighten the
Dijkstra comparison: replace the single-term comparison "C << log V" with a
comparison of full asymptotic costs, e.g., "Dial's algorithm runs in O(V*C + E)
time vs Dijkstra with a heap in O((V+E) log V), so use Dial when V*C + E <<
(V+E) log V (equivalently V*C << (V+E) log V)"; also add a short practical rule
of thumb (one sentence) suggesting Dial is especially effective on sparse graphs
(small E) with small integer edge weights (small C).
In `@README.md`:
- Around line 258-260: The ASCII table row for "0-1 BFS / Dial / Bidirectional
BFS / IDA*" is missing the left vertical border/indentation, causing
misalignment; update the three rows containing "A* Search", "0-1 BFS / Dial /
Bidirectional BFS / IDA*", and "Biconnected"/"Cut Nodes/Bridges" so each line
begins with the same "│" frame prefix and matching spacing, ensuring the left
column entries ("A* Search", "0-1 BFS / Dial / Bidirectional BFS / IDA*") and
the right column entries ("Cut Nodes/Bridges", "Biconnected") are vertically
aligned within the box; after fixing the prefix, optionally run a monospace
alignment pass across adjacent rows to normalize spacing.
In `@Tests/test_bidirectional_bfs.cc`:
- Around line 358-360: Replace the std::vector<Node *> nodes(N) usage with the
Aleph container Array<Node *> (e.g., Array<Node *> nodes(N)) in both places
where nodes is created (the earlier loop using g.insert_node and the later
occurrence around lines mentioned in the review), initialize the Array elements
explicitly to nullptr before filling them for clarity, and ensure this Aleph
container convention is used for all new shortest-path tests; locate and update
the declarations and any uses of the nodes variable and the loop that calls
g.insert_node(i) accordingly.
In `@Tests/test_dial.cc`:
- Around line 381-383: Replace the std::vector-based node buffer with the Aleph
container: change the declaration of nodes from std::vector<Node *> to
Aleph::Array<Node *> (or Array<Node *>) and initialize it explicitly to size N
before filling (e.g., allocate/construct the Aleph array of length N and then
assign nodes[i] = g.insert_node(i)); apply the same replacement and explicit
initialization pattern to the other block referenced (lines ~631-633) so all new
graph algorithm tests use Aleph containers instead of std::vector.
---
Nitpick comments:
In `@Zero_One_BFS.H`:
- Around line 361-394: The get_distance method recomputes path length by walking
parents because uninit_paint() overwrites NODE_COOKIE with only the parent
pointer; fix by preserving the precomputed distance in NODE_COOKIE during
painting/uninit_paint: introduce a small struct (e.g., Painted_Info) that holds
both parent pointer and the Node_Info::dist value, update the painting code and
uninit_paint to store that struct in NODE_COOKIE, and change get_distance to
return static_cast<Painted_Info*>(NODE_COOKIE(node))->dist for O(1) access while
keeping existing parent traversal logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 063148c4-4743-4493-ac94-20c709b7fe76
📒 Files selected for processing (13)
Bidirectional_BFS.HDial.HExamples/bidirectional_bfs_example.ccExamples/ida_star_example.ccExamples/zero_one_bfs_example.ccIDA_Star.HREADME.mdTests/run_one_test.rbTests/test_bidirectional_bfs.ccTests/test_dial.ccTests/test_ida_star.ccTests/test_zero_one_bfs.ccZero_One_BFS.H
🚧 Files skipped from review as they are similar to previous changes (2)
- Examples/ida_star_example.cc
- Examples/bidirectional_bfs_example.cc
Update Bidirectional_BFS.H, Dial.H, Zero_One_BFS.H, and tpl_interval_tree.H to improve documentation, RAII guards, and node information handling. Refactor node array creation in test files to use `Aleph::Array<Node *>::create(N)` instead of `std::vector<Node *> nodes(N)` for consistency with Aleph's internal `Array` usage. Enhance `Zero_One_BFS` to store `Painted_Info` directly, enabling O(1) distance access. Add detailed Doxygen comments for clarity on BiBFS and Dial's algorithm internals. The `README.md` was also significantly updated.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Zero_One_BFS.H (1)
216-230:⚠️ Potential issue | 🟠 Major
init()can leak partially allocated node metadata on allocation failure.Must fix: The guard is created after
init(g)returns, so ifnew Node_Infothrows insideinit, already allocated cookies are not reclaimed.🛡️ Proposed fix
void init(const GT & g) { ptr_g = &const_cast<GT &>(g); @@ - for (typename GT::Node_Iterator it(g); it.has_curr(); it.next()) - NODE_COOKIE(it.get_curr()) = new Node_Info; + try + { + for (typename GT::Node_Iterator it(g); it.has_curr(); it.next()) + NODE_COOKIE(it.get_curr()) = new Node_Info; + } + catch (...) + { + uninit_discard(); + reset_state_after_failure(); + throw; + } }As per coding guidelines: Ensure exception safety (basic, strong, or no-throw guarantee).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Zero_One_BFS.H` around lines 216 - 230, init currently does per-node raw new for Node_Info and can leak if one allocation throws; change the allocation strategy to be exception-safe: allocate Node_Info objects into a local RAII container (e.g., std::vector<std::unique_ptr<Node_Info>> or similar) while iterating with GT::Node_Iterator, and only after all allocations succeed assign/transfer ownership into NODE_COOKIE for each node (or swap pointers) and set ptr_g as now; alternatively wrap the allocation loop in a try/catch that deletes any already-allocated NODE_COOKIE entries on failure—use the symbols init, Node_Info, NODE_COOKIE, ptr_g and GT::Node_Iterator to locate and update the code.
🧹 Nitpick comments (2)
Tests/test_ida_star.cc (2)
151-161: Should fix: extend precondition tests beyond null-node inputs.This block validates null nodes, but contract-level preconditions for empty graphs and negative weights are still untested. Adding those cases here would better protect API behavior.
Based on learnings: "Tests must cover edge cases: empty inputs, single element, large inputs" and "Validate input preconditions (e.g., non-negative weights for Dijkstra)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/test_ida_star.cc` around lines 151 - 161, Extend the IDAStar.NullNodeValidation test to also assert contract preconditions for empty graphs and negative weights: create an empty GT (no nodes) and call ida(g_empty, nullptr or any node pointer, nullptr, path) or ida(g_empty, n0, n0, path) expecting std::domain_error; also construct a GT with an edge that has a negative weight and call IDA_Zero<GT>::operator()(g_neg, start, goal, path) expecting std::domain_error (use the existing TEST name IDAStar.NullNodeValidation, GT, IDA_Zero<GT>, ida(...), Path<GT>, and n0 to locate where to add these new EXPECT_THROW assertions).
593-602: Should fix:ChebyshevHeuristicExistsonly instantiates the type, not behavior.Right now it won’t catch implementation regressions inside
operator(). A tiny value check would make this test meaningful.Proposed improvement
TEST(IDAStar, ChebyshevHeuristicExists) { - // Just verify the template instantiates struct Coord { int x; int y; }; using CG = List_Graph<Graph_Node<Coord>, Graph_Arc<int>>; using CH = Chebyshev_Heuristic<CG>; - CH ch; - (void)ch; // suppress unused warning + CG g; + auto a = g.insert_node({0, 0}); + auto b = g.insert_node({2, 1}); + CH ch; + EXPECT_EQ(ch(a, b), 2); }Based on learnings: "Tests must verify algorithmic correctness, not just absence of crashes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/test_ida_star.cc` around lines 593 - 602, The test ChebyshevHeuristicExists currently only instantiates Chebyshev_Heuristic<CG> but doesn't exercise operator(); update the test to create two Graph_Node<Coord> (or nodes with Coord {x,y}) with known coordinates, call CH::operator()(nodeA, nodeB) (i.e., ch(nodeA, nodeB)) and add an EXPECT_EQ/ASSERT_EQ that the returned heuristic equals the expected Chebyshev distance (max(|dx|,|dy|)) for those coordinates so the test validates actual behavior rather than just instantiation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dial.H`:
- Around line 435-440: The Doxygen `@throw` lists for public functions
paint_min_paths_tree() and find_min_path() are incomplete: update their
documentation to include all exceptions that can be raised by run_dial(),
specifically add overflow/runtime errors (e.g., distance-range checks and
bucket-limit guards) in addition to domain_error and bad_alloc; ensure each
public member has `@brief`, `@param`, `@return`, and `@throws` entries and mention the
exact exception types (domain_error, bad_alloc, overflow_error/runtime_error as
applicable) so callers see all possible throws for paint_min_paths_tree(),
find_min_path(), and any function that forwards run_dial() errors.
- Around line 479-496: The loop in get_distance() can accumulate a partial total
when a null parent is encountered or when no visited parent arc is found; modify
the traversal in the for loop that uses NODE_COOKIE, Node, Itor<GT, SA>,
IS_ARC_VISITED(Aleph::Spanning_Tree) and distance() so that if parent == nullptr
OR the inner Itor scan does not find any arc with IS_ARC_VISITED(arc,
Aleph::Spanning_Tree) leading to curr, the function aborts the reconstruction
(e.g., return a sentinel like INF or a failure indicator / throw) instead of
breaking or continuing, ensuring total is not returned as a truncated partial
distance. Ensure callers of get_distance() handle this sentinel/failure
appropriately.
In `@Tests/test_ida_star.cc`:
- Line 166: The test name has a typo: rename the GoogleTest case identifier
TEST(IDAStar, RelaxationFindsCheeperPath) to TEST(IDAStar,
RelaxationFindsCheaperPath) so the test name reads "Cheaper" instead of
"Cheeper"; update any occurrences of RelaxationFindsCheeperPath (test
declaration and any references) to RelaxationFindsCheaperPath to keep names
consistent.
In `@Zero_One_BFS.H`:
- Around line 206-213: The rollback path can delete objects as Node_Info* if new
Painted_Info throws mid-conversion because some nodes are already converted
while painted==false; to fix, make the conversion two-phase in
uninit_paint()/the conversion loop: first allocate/construct each Painted_Info
(or a temporary array/ownership wrapper) without mutating the live node
pointers, only after successful allocation of the new Painted_Info for a node
atomically replace the Node_Info pointer and then set painted=true (or set
painted only after all nodes are replaced), and ensure
ZOB_Init_Guard::~ZOB_Init_Guard()/owner->uninit_discard()/owner->reset_state_after_failure()
treat partially-converted nodes correctly by cleaning up any allocated
Painted_Info via the temporary ownership wrapper rather than DELETEing them as
Node_Info; use RAII (unique_ptr/placement-new + destructor) to guarantee no
type-mismatched deletes on exceptions.
- Around line 435-448: The function get_min_path dereferences end via
IS_NODE_VISITED without validating end; add an explicit null check for end right
after the painted and ptr_g checks (before any use of end) — e.g. use the
existing error-checking macro to fail fast: ah_domain_error_if(end == nullptr)
<< "end pointer is null"; alternatively, if the API expects a non-exception
return, return Inf after ensuring path remains empty; ensure the check appears
before IS_NODE_VISITED(end, Aleph::Spanning_Tree) and reference symbols:
get_min_path, end, IS_NODE_VISITED, path, s, ptr_g, painted.
- Around line 359-374: The method paint_min_paths_tree overwrites prior run
state (NODE_COOKIE allocations and node marks) by calling init(g) without first
releasing previous metadata; add a cleanup step that frees any existing
NODE_COOKIE allocations and clears painted/node marks before reinitializing.
Concretely, ensure paint_min_paths_tree (and the code paths around lines where
init(g) is called) invokes uninit_paint() or a new discard_previous_state() that
releases NODE_COOKIEs and resets per-node markers, or guard against double-init
inside init() by calling that cleanup when s != nullptr or painted == true; then
proceed with ZOB_Init_Guard, run_bfs, and set s/painted as before. Reference
symbols: paint_min_paths_tree, init, uninit_paint (or create
discard_previous_state), NODE_COOKIE, s, painted, run_bfs, ZOB_Init_Guard.
---
Duplicate comments:
In `@Zero_One_BFS.H`:
- Around line 216-230: init currently does per-node raw new for Node_Info and
can leak if one allocation throws; change the allocation strategy to be
exception-safe: allocate Node_Info objects into a local RAII container (e.g.,
std::vector<std::unique_ptr<Node_Info>> or similar) while iterating with
GT::Node_Iterator, and only after all allocations succeed assign/transfer
ownership into NODE_COOKIE for each node (or swap pointers) and set ptr_g as
now; alternatively wrap the allocation loop in a try/catch that deletes any
already-allocated NODE_COOKIE entries on failure—use the symbols init,
Node_Info, NODE_COOKIE, ptr_g and GT::Node_Iterator to locate and update the
code.
---
Nitpick comments:
In `@Tests/test_ida_star.cc`:
- Around line 151-161: Extend the IDAStar.NullNodeValidation test to also assert
contract preconditions for empty graphs and negative weights: create an empty GT
(no nodes) and call ida(g_empty, nullptr or any node pointer, nullptr, path) or
ida(g_empty, n0, n0, path) expecting std::domain_error; also construct a GT with
an edge that has a negative weight and call IDA_Zero<GT>::operator()(g_neg,
start, goal, path) expecting std::domain_error (use the existing TEST name
IDAStar.NullNodeValidation, GT, IDA_Zero<GT>, ida(...), Path<GT>, and n0 to
locate where to add these new EXPECT_THROW assertions).
- Around line 593-602: The test ChebyshevHeuristicExists currently only
instantiates Chebyshev_Heuristic<CG> but doesn't exercise operator(); update the
test to create two Graph_Node<Coord> (or nodes with Coord {x,y}) with known
coordinates, call CH::operator()(nodeA, nodeB) (i.e., ch(nodeA, nodeB)) and add
an EXPECT_EQ/ASSERT_EQ that the returned heuristic equals the expected Chebyshev
distance (max(|dx|,|dy|)) for those coordinates so the test validates actual
behavior rather than just instantiation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d96940fb-8fbb-439b-9550-a3e53d23976f
📒 Files selected for processing (9)
Bidirectional_BFS.HDial.HREADME.mdTests/test_bidirectional_bfs.ccTests/test_dial.ccTests/test_ida_star.ccTests/test_zero_one_bfs.ccZero_One_BFS.Htpl_interval_tree.H
Add detailed error checks for IDA* and Dial algorithms. Improve Zero_One_BFS memory management for node cookies. Add IDA* Chebyshev heuristic test.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Zero_One_BFS.H`:
- Around line 367-368: The computation of new_dist in Zero_One_BFS (new_dist =
curr_dist + w) can overflow for small Distance_Type; add a guard like the other
algorithms (Dial.H, Bellman_Ford.H): check that curr_dist is not the sentinel
INF and that curr_dist <= numeric_limits<Distance_Type>::max() - w before
performing the addition, and treat cases that would overflow as "infinite"
(skip/update appropriately) so distances aren't silently corrupted; update
references to new_dist/curr_dist/w in Zero_One_BFS accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a07da1ec-6bc8-4401-ba32-81bf7c63aa54
📒 Files selected for processing (5)
Dial.HIDA_Star.HTests/test_ida_star.ccTests/test_zero_one_bfs.ccZero_One_BFS.H
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
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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
Bug Fixes