feat: bfs dijkstra flow map#97
Conversation
📝 WalkthroughWalkthroughThis PR adds BFS (breadth-first) pathfinding to the ChangesBFS Feature Implementation
Dijkstra Algorithm Correctness Fix
Package Documentation Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
Malibu-With-Coke
force-pushed
the
feat/bfs-dijkstra-flow-map
branch
from
4d8ab73 to
bf96536
Compare
![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
🧹 Nitpick comments (3)
bracket-pathfinding/src/bfs.rs (3)
249-249: 💤 Low value
partial_cmp().unwrap()will panic if BFS map contains NaN.While the map is initialized to
f32::MAX(safe for comparison), the publicmapfield allows external modification. If corrupted with NaN,partial_cmpreturnsNoneand the unwrap panics.Consider using
unwrap_or(Ordering::Equal)for defensive handling:🛡️ Suggested fix
- exits.par_sort_by(|a, b| bfs.map[a.0].partial_cmp(&bfs.map[b.0]).unwrap()); + exits.par_sort_by(|a, b| { + bfs.map[a.0] + .partial_cmp(&bfs.map[b.0]) + .unwrap_or(std::cmp::Ordering::Equal) + });Apply similarly to all four helper variants.
Also applies to: 262-262
🤖 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 `@bracket-pathfinding/src/bfs.rs` at line 249, The sort comparator currently calls bfs.map[a.0].partial_cmp(&bfs.map[b.0]).unwrap(), which will panic if either value is NaN; change the comparator to handle None defensively by using .unwrap_or(std::cmp::Ordering::Equal) (or Ordering::Equal via a use) instead of unwrap so NaNs don't cause a panic; replace the unwrap in exits.par_sort_by and the three other helper variants that use partial_cmp on bfs.map with this defensive pattern referencing the exits.par_sort_by call and the bfs.map field.
298-324: ⚡ Quick winConsider expanding test coverage for BFS module.
The existing test validates that BFS counts edges rather than costs, which is good. However, coverage is minimal for a new module. Consider adding tests for:
find_lowest_exit/find_highest_exithelpersmax_depthcutoff behavior (verify tiles beyond cutoff remainf32::MAX)new_emptyandclearmethods- Parallel build path (under
#[cfg(feature = "threaded")])As per coding guidelines: "Add tests for new functionality in the relevant module; for split domains, prefer colocated
tests.rs".🤖 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 `@bracket-pathfinding/src/bfs.rs` around lines 298 - 324, Add unit tests in the BFS test module to increase coverage: write tests that call find_lowest_exit and find_highest_exit on small maps with known exits to assert correct indices are returned; test max_depth cutoff by creating a BfsMap with a tight max_depth and assert tiles beyond the cutoff remain f32::MAX; add tests for BfsMap::new_empty (or new_empty) and BfsMap::clear to verify they initialize and reset the internal map state as expected; and, if the threaded feature exists, add a test exercising the parallel build path (e.g., BfsMap::new with threaded feature) to ensure it produces identical results to the single-threaded build. Ensure each test references the BfsMap methods and helper functions by name so they’re easy to locate.
204-210: 💤 Low valueParallel BFS hardcodes start depth to 0.0, unlike serial
build_weighted.The serial
build_weightedaccepts(usize, f32)tuples allowing variable start depths, but the parallel path hardcodes0.0for all starts. The condition0.0 >= l.map[start]is always false (sincel.map[start]isf32::MAX), making the first check dead code.Currently safe since
build_parallelis only invoked frombuild()which uses depth 0.0 for all starts. Consider a TODO if future support for parallel weighted starts is desired.🤖 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 `@bracket-pathfinding/src/bfs.rs` around lines 204 - 210, The parallel BFS currently hardcodes start depth to 0.0 and leaves a dead branch; update build_parallel (and any callers that populate l.starts) to accept start depth tuples like the serial build_weighted by changing l.starts to hold (usize, f32) pairs, then in the loop use the provided depth (e.g., for (start, depth) in l.starts.iter().copied()) and replace the checks/assignments to compare and assign depth (if depth >= l.map[start] || depth >= l.max_depth { continue } l.map[start] = depth; open_list.push_back((start, depth));). Also update any call sites that construct l.starts and add a TODO comment if you prefer to postpone full API changes.
🤖 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 `@manual/src/ex_path.md`:
- Line 39: Replace the empty alt text in the image markdown
 with a concise, descriptive alt string that explains the
image content for accessibility (for example: );
locate the image token in manual/src/ex_path.md and update only the alt text
between the brackets to a brief descriptive phrase that conveys the image's
meaning to screen readers.
---
Nitpick comments:
In `@bracket-pathfinding/src/bfs.rs`:
- Line 249: The sort comparator currently calls
bfs.map[a.0].partial_cmp(&bfs.map[b.0]).unwrap(), which will panic if either
value is NaN; change the comparator to handle None defensively by using
.unwrap_or(std::cmp::Ordering::Equal) (or Ordering::Equal via a use) instead of
unwrap so NaNs don't cause a panic; replace the unwrap in exits.par_sort_by and
the three other helper variants that use partial_cmp on bfs.map with this
defensive pattern referencing the exits.par_sort_by call and the bfs.map field.
- Around line 298-324: Add unit tests in the BFS test module to increase
coverage: write tests that call find_lowest_exit and find_highest_exit on small
maps with known exits to assert correct indices are returned; test max_depth
cutoff by creating a BfsMap with a tight max_depth and assert tiles beyond the
cutoff remain f32::MAX; add tests for BfsMap::new_empty (or new_empty) and
BfsMap::clear to verify they initialize and reset the internal map state as
expected; and, if the threaded feature exists, add a test exercising the
parallel build path (e.g., BfsMap::new with threaded feature) to ensure it
produces identical results to the single-threaded build. Ensure each test
references the BfsMap methods and helper functions by name so they’re easy to
locate.
- Around line 204-210: The parallel BFS currently hardcodes start depth to 0.0
and leaves a dead branch; update build_parallel (and any callers that populate
l.starts) to accept start depth tuples like the serial build_weighted by
changing l.starts to hold (usize, f32) pairs, then in the loop use the provided
depth (e.g., for (start, depth) in l.starts.iter().copied()) and replace the
checks/assignments to compare and assign depth (if depth >= l.map[start] ||
depth >= l.max_depth { continue } l.map[start] = depth;
open_list.push_back((start, depth));). Also update any call sites that construct
l.starts and add a TODO comment if you prefer to postpone full API changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3cf71e9-216e-480e-8afa-cd5ffeb3d125
📒 Files selected for processing (9)
bracket-pathfinding/Cargo.tomlbracket-pathfinding/README.mdbracket-pathfinding/examples/bfs/main.rsbracket-pathfinding/src/bfs.rsbracket-pathfinding/src/dijkstra.rsbracket-pathfinding/src/lib.rsmanual/src/ex_path.mdmanual/src/individual_parts.mdsrc/lib.rs
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format all Rust code usingcargo fmt --all
Fix allcargo clippywarnings — the CI enforces-D warnings
Add tests for new functionality in the relevant module; for split domains, prefer colocatedtests.rs
Files:
bracket-pathfinding/examples/bfs/main.rsbracket-pathfinding/src/lib.rssrc/lib.rsbracket-pathfinding/src/dijkstra.rsbracket-pathfinding/src/bfs.rs
🪛 LanguageTool
manual/src/individual_parts.md
[style] ~9-~9: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...erating things. * bracket-pathfinding provides A-Star, BFS and Dijkstra mapping soluti...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
bracket-pathfinding/README.md
[style] ~3-~3: ‘in conjunction with’ might be wordy. Consider a shorter alternative.
Context: ... the overall bracket-lib system, and (in conjunction with bracket-algorithm-traits) provides pa...
(EN_WORDINESS_PREMIUM_IN_CONJUNCTION_WITH)
🪛 markdownlint-cli2 (0.22.1)
manual/src/ex_path.md
[warning] 39-39: Images should have alternate text (alt text)
(MD045, no-alt-text)
🔇 Additional comments (18)
bracket-pathfinding/Cargo.toml (1)
7-7: LGTM!src/lib.rs (1)
40-40: LGTM!bracket-pathfinding/README.md (1)
3-3: LGTM!Also applies to: 29-29, 91-107, 121-121, 137-137, 141-141, 144-147
manual/src/ex_path.md (1)
19-24: LGTM!manual/src/individual_parts.md (1)
9-9: LGTM!bracket-pathfinding/src/bfs.rs (5)
267-295: Samepartial_cmp().unwrap()consideration applies here as noted above.
1-40: LGTM!
42-110: LGTM!
112-135: LGTM!
137-179: LGTM!bracket-pathfinding/src/lib.rs (1)
4-4: LGTM!Also applies to: 10-10
bracket-pathfinding/examples/bfs/main.rs (1)
1-47: LGTM!bracket-pathfinding/src/dijkstra.rs (6)
6-7: LGTM!The
DijkstraNodeordering correctly implements min-heap semantics by reversing the cost comparison (other.cost.partial_cmp(&self.cost)), which is the standard pattern for Dijkstra with Rust's max-heapBinaryHeap. Tie-breaking by index ensures deterministic behavior.Also applies to: 29-57
178-179: LGTM!
185-219: LGTM!The priority-queue Dijkstra implementation is correct: seeds starts into the heap, pops minimum-cost nodes, skips stale entries, and relaxes neighbors with proper cost accumulation. This fixes the previous BFS-like behavior that could produce incorrect results with non-uniform exit costs.
223-286: LGTM!The parallel implementation correctly uses per-layer
BinaryHeapwith proper stale-entry checks and min-recombination. Pre-computing exits outside the parallel loop is a good optimization to avoid redundantget_available_exitscalls.
382-392: LGTM!
457-466: LGTM!This test validates the core correctness fix: with the priority-queue implementation, Dijkstra correctly discovers that path 0→2→1 (cost 2.0) is cheaper than direct path 0→1 (cost 10.0). This was the exact scenario that would fail with the previous BFS-like traversal.
| Demonstrates the Field-of-View functionality. | ||
|
|
||
|  No newline at end of file | ||
|  |
There was a problem hiding this comment.
Add alt text to the image markdown for accessibility.
Line 39 should use descriptive alt text instead of an empty alt field.
Proposed fix
-
+📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
|  | |
|  |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 39-39: Images should have alternate text (alt text)
(MD045, no-alt-text)
🤖 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 `@manual/src/ex_path.md` at line 39, Replace the empty alt text in the image
markdown  with a concise, descriptive alt string that
explains the image content for accessibility (for example: ); locate the image token in manual/src/ex_path.md and
update only the alt text between the brackets to a brief descriptive phrase that
conveys the image's meaning to screen readers.
1 participant
What
Add a dedicated BFS flow map, update Dijkstra maps to use a priority queue for weighted shortest-path costs, and add BFS example/docs coverage.
Why
The previous
DijkstraMapimplementation used FIFO queue traversal, which behaves more like BFS/flood fill and can produce incorrect results with non-uniform exit costs. Splitting BFS intoBfsMapand movingDijkstraMaptoBinaryHeapkeeps the algorithm roles clear and makes weighted path costs correct.Closes #32
Checklist
Required
cargo check --allpassescargo fmt --all -- --checkpassescargo clippy --workspace --all-targets -- -D warnings -A clippy::multiple-crate-versionspassescargo test --allpassesCloses #123)Functional Validation
Configuration & Docs
README.md,ARCHITECTURE.md, or relevant manual pages, if applicable)If Applicable
cargo auditlocally if needed)Summary by CodeRabbit
New Features
Bug Fixes
Documentation