Extract common search engine logic and add incremental evaluator support'#51
Merged
Extract common search engine logic and add incremental evaluator support'#51
Conversation
Refactor common helper utilities used internally by all search engines into `search_engine_detail`. This eliminates duplicated logic for `is_terminal_state`, `should_prune_state`, `register_visit`, `expansion_limit_reached`, and `stop_after_solution`. This is a very complex change, leading to a significant refactoring of core search logic.
Introduce exception-safety for adversarial search apply/undo to ensure correct state restoration. Add new concepts for incremental bound/evaluation providers. Add `SupportsBestFirst` concept and update `ExplorationPolicy` documentation. refactor(test): Update search framework tests Introduce new test cases for: - Adversarial search: exception safety, forced draws, empty successors, extreme scores. - Branch and bound: max depth/expansions limits, zero knapsack capacity. - IDA* and DFS backtracking: `SearchStorageSet` concept, no path to goal, inadmissible heuristics. - N-Queens for N=0 and N=1. - `SearchStorageSet` basic operations. docs(README): Update search framework overview Reflect the new `IDA_Star.H` engine, `SearchStorageSet`, and updated capabilities for move ordering, transposition tables, and iterative deepening. Update test command examples.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralized shared search-control helpers, added incremental move-estimate concepts and engine capability flags, generalized TT/iterative-deepening templates, improved apply/evaluate/undo exception-safety, added IDA* and move-ordering utilities, and expanded examples and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Engine
participant Domain
participant TT as TranspositionTable
participant Tracer
Client->>Engine: run_iterative_deepening(initial_state, options, tracer)
Engine->>Tracer: iteration_begin(depth)
Engine->>TT: probe(state_key)
TT-->>Engine: entry or miss
Engine->>Domain: is_terminal_state(state)
alt not terminal
Engine->>Domain: for_each_successor -> move
alt IncrementalEvaluator available
Engine->>Domain: evaluate_after(state, move)
else
Engine->>Domain: apply(move)
Engine->>Domain: evaluate(state')
Engine->>Domain: undo(move)
end
Engine->>Engine: recurse/search_node(...)
Engine->>TT: store_adversarial_transposition(entry)
end
Engine->>Tracer: iteration_end(depth, result)
Engine-->>Client: final_result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the search-framework work by adding new domain/engine concepts, consolidating shared engine bookkeeping into a single helper namespace, improving exception safety around apply/undo patterns, and expanding test coverage across backtracking, IDA*, branch-and-bound, and adversarial search.
Changes:
- Introduces incremental move-estimation concepts (
IncrementalBoundProvider,IncrementalEvaluator) and an engine capability concept (SupportsBestFirst), plus sharedsearch_engine_detailhelpers. - Refactors Backtracking / IDA* / Branch-and-Bound / Negamax / Alpha-Beta to reuse shared helpers and to better handle exceptions during
apply()/recursion/ordering. - Adds multiple new tests covering visited-set contracts, IDA* edge cases, B&B limit behaviors, and adversarial exception-safety and corner cases; updates README test instructions.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| state_search_common.H | Adds new concepts and centralizes common helper logic for engines (terminal/prune dispatch, visit/limit bookkeeping). |
| search_move_ordering.H | Documents no-op history heuristic table behavior more explicitly. |
| State_Search_IDA_Star.H | Uses shared helpers and introduces an unreachable-distance sentinel utility for clearer IDA* bookkeeping. |
| Backtracking.H | Replaces duplicated pruning/limit logic with search_engine_detail helpers. |
| Branch_And_Bound.H | Adds compile-time supports_best_first, exception-safe apply_move_and_recurse, and incremental bound ordering fast-path. |
| Negamax.H | Adds supports_best_first, factors TT probe/store + iterative deepening into shared helpers, and improves exception safety around apply/undo. |
| Alpha_Beta.H | Adds supports_best_first, incremental evaluator fast-path for ordering, shared TT helpers + iterative deepening loop reuse, and exception safety around apply/undo. |
| Tests/state_search_framework_test.cc | Adds visited-set assertions and new IDA* edge-case tests. |
| Tests/branch_and_bound_test.cc | Adds tests for depth/expansion limits and knapsack capacity=0. |
| Tests/adversarial_search_test.cc | Adds adversarial exception-safety tests and additional edge-case coverage (forced draw, empty successors, extreme scores). |
| README.md | Updates “what’s available” list and test build/run instructions. |
![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: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Negamax.H`:
- Around line 663-668: The shared helper store_adversarial_transposition()
currently increments stores and replacements based on table.store() but does not
account for TranspositionStoreStatus::Rejected; update the helper so that when
const auto status = table.store(typename Table::Key_Type{keyer(state),
remaining}, entry); yields TranspositionStoreStatus::Rejected you increment
result.stats.transpositions.rejected_updates (in the same area where stores and
replacements are updated), keeping existing increments for stores and
replacements intact.
- Around line 769-788: The code promotes an iteration as completed even when it
hit a search limit because out.result, out.iterations.append(iteration), and
++out.completed_iterations occur before checking
iteration.result.limit_reached() or depth == limits.max_depth. Fix by moving or
gating the promotion logic: first call accumulate_adversarial_stats and
emit_trace as needed, then check if iteration.result.limit_reached() or depth ==
limits.max_depth and break without mutating out; only after passing that check
assign out.result = iteration.result, append to out.iterations, and increment
out.completed_iterations so interrupted/limit-hit iterations are not reported as
completed.
In `@README.md`:
- Around line 3621-3625: The README incorrectly tells Backtracking.H users to
use SearchStorageSet; update the text to reference the actual depth-aware
visited container exposed by Backtracking.H—VisitedBoundMap<..., size_t>—and
only mention SearchStorageSet where the facade that provides the set-based flow
is documented (call out that facade by name if present). Concretely, replace the
SearchStorageSet reference in the Backtracking.H bullet with
VisitedBoundMap<..., size_t> and add a short parenthetical that the set-based
flow is available via the other facade (name that facade) rather than directly
from Backtracking.H.
- Around line 3629-3631: The MD037 lint error is caused by the bare asterisk in
the text "IDA*" — update the README occurrences of "IDA*" (look for the
"State_Search_IDA_Star.H" line and any other mentions) to escape the asterisk as
"IDA\*" so the Markdown linter stops treating it as emphasis; ensure both
occurrences are updated consistently.
In `@state_search_common.H`:
- Around line 179-181: The documentation for IncrementalBoundProvider currently
requires bound_after(state, move) to be an “optimistic upper bound”, which is
max-only and wrong for minimization; update the doc for
IncrementalBoundProvider::bound_after (and any related comments near the
declaration) to be objective-policy-neutral by stating it must return a value
convertible to Domain::Objective that represents an optimistic bound in the
direction of the current ObjectivePolicy (i.e., an upper bound for maximization
and a lower bound for minimization), and mention Branch_And_Bound and
Minimize_Objective so implementers know to flip the bound sign/semantics
accordingly.
- Around line 741-748: The helper is_terminal_state passes a const Domain& to
domain.is_terminal but the TerminalPredicate concept currently accepts a
non-const Domain, causing spurious compile errors; to fix, update the concept
TerminalPredicate so it requires that domain.is_terminal is callable on a const
Domain& (i.e., change the concept's callable check to accept const Domain& and
the typename Domain::State), so is_terminal_state can continue passing a const
Domain& to domain.is_terminal without errors—adjust the concept definition that
mentions TerminalPredicate (and any associated requires-expression) rather than
changing is_terminal_state or is_terminal usage.
In `@State_Search_IDA_Star.H`:
- Around line 115-131: The header is missing the <type_traits> include used by
std::is_floating_point_v in the template function distance_unreachable(); add a
direct include for <type_traits> at the top of the header (alongside the
existing <limits> and <utility>) so the use of std::is_floating_point_v is not
reliant on transitive includes and the header becomes self-sufficient.
In `@Tests/adversarial_search_test.cc`:
- Around line 1124-1241: The tests never exercise the new IncrementalEvaluator
fast path because they all use the legacy apply()+evaluate() ordering; update or
add tests to exercise Alpha_Beta::collect_ordered_moves()'s IncrementalEvaluator
branch by providing a game domain or evaluator that implements evaluate_after()
(or the incremental evaluator interface) so ordering is done via
evaluate_after() instead of apply()/evaluate(); specifically create a
fixture/domains (similar to ThrowingPostApplyGameDomain or
EmptySuccessorGameDomain) that implements evaluate_after() semantics and add
parallel tests (for Negamax and Alpha_Beta/search helpers) that call
negamax_search/alpha_beta_search (or directly Alpha_Beta::collect_ordered_moves)
to assert correct behavior and exception/leaf handling when using
IncrementalEvaluator.
In `@Tests/branch_and_bound_test.cc`:
- Around line 909-958: The tests never exercise the IncrementalBoundProvider
path in Branch_And_Bound::collect_ordered_moves(), so add a new unit test that
uses a tiny domain implementing bound_after(state, move) (and a corresponding
apply() that increments a counter) to verify the incremental bound branch is
taken and that moves are ordered without mutating the original state;
specifically create a fixture/domain that overrides bound(state) and
bound_after(state, move), tracks apply() calls, invoke branch_and_bound_search
(or Branch_And_Bound<YourDomain>::search) and assert that bound_after was
invoked (via the counter) and the search behavior matches expected
ordering/solution behavior so the IncrementalBoundProvider branch is covered.
In `@Tests/state_search_framework_test.cc`:
- Around line 1416-1539: The test domains (NoPathIDADomain and
InadmissibleHeuristicDomain) still use integral Distance so the new
floating-point path in distance_unreachable() is never exercised; change their
using Distance = int to using Distance = double (or float) so the floating-point
branch runs, and tighten the assertion on the inadmissible-heuristic test:
replace EXPECT_GT(result.total_cost, 0) with a precise check (e.g.,
EXPECT_DOUBLE_EQ(result.total_cost, 2.0) or EXPECT_NEAR(..., 2.0, small_eps)) to
detect threshold/path-cost regressions. Ensure to update any related comparisons
in the test to use the appropriate floating-point EXPECT_* matcher.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fea92ee-3287-4775-b828-6e5ddf18ed8c
📒 Files selected for processing (11)
Alpha_Beta.HBacktracking.HBranch_And_Bound.HNegamax.HREADME.mdState_Search_IDA_Star.HTests/adversarial_search_test.ccTests/branch_and_bound_test.ccTests/state_search_framework_test.ccsearch_move_ordering.Hstate_search_common.H
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Tests/state_search_framework_test.cc (1)
1543-1552:⚠️ Potential issue | 🟡 MinorShould fix — assert the floating-point unreachable sentinel in the new IDA* regression.
Switching this fixture to
doubleonly exercises the new branch; it still does not prove that the last pass returned the floating-point unreachable sentinel. Ifdistance_unreachable<double>()regresses frominfinity()back tomax(), this test stays green because it only checksexhausted().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/state_search_framework_test.cc` around lines 1543 - 1552, The test currently only checks exhausted() but must also assert that the final/last-pass distance is the floating-point unreachable sentinel; after calling IDA_Star_State_Search::search and getting result, add an assertion that the search result's final bound/last-pass distance (e.g., result.last_pass_distance() or the result field/method that reports the last pass distance) is equal to distance_unreachable<double>() (use the exact sentinel, not an integer max). This ensures the new double-based IDA* branch returns the floating-point unreachable value rather than an integer max() fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@State_Search_IDA_Star.H`:
- Around line 116-132: The code introduced distance_unreachable<Distance>() but
exhaustion detection still compares next_bound to
std::numeric_limits<Distance>::max(); update the check(s) that treat a bound of
"max()" as unreachable (e.g., the comparison involving next_bound around the
IDA* loop) to instead compare against distance_unreachable<Distance>() (or use a
helper like is_unreachable(next_bound) that calls distance_unreachable) so
floating-point infinity is correctly recognized as the sentinel; search for
occurrences of std::numeric_limits<Distance>::max() used as the unreachable
sentinel (including the check near next_bound) and replace them to use
distance_unreachable<Distance>() or the helper.
---
Duplicate comments:
In `@Tests/state_search_framework_test.cc`:
- Around line 1543-1552: The test currently only checks exhausted() but must
also assert that the final/last-pass distance is the floating-point unreachable
sentinel; after calling IDA_Star_State_Search::search and getting result, add an
assertion that the search result's final bound/last-pass distance (e.g.,
result.last_pass_distance() or the result field/method that reports the last
pass distance) is equal to distance_unreachable<double>() (use the exact
sentinel, not an integer max). This ensures the new double-based IDA* branch
returns the floating-point unreachable value rather than an integer max()
fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 228f7a20-efb7-4a1b-afa9-b0a66c8c9886
📒 Files selected for processing (8)
Alpha_Beta.HNegamax.HREADME.mdState_Search_IDA_Star.HTests/adversarial_search_test.ccTests/branch_and_bound_test.ccTests/state_search_framework_test.ccstate_search_common.H
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- Negamax.H
Improve comments for store and probe transposition functions. Correct reference to Depth_First_Backtracking in state_search_common.H.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
state_search_common.H (1)
818-849: Document the call-order precondition forstop_after_solution().This helper only enforces
max_solutionscorrectly ifresult.stats.solutions_foundalready includes the newly accepted solution. A short@prewould make that shared contract explicit for future engine call sites.As per coding guidelines, "Document preconditions (`@pre`) and postconditions (`@post`) when relevant."📝 Suggested wording
/** `@brief` Decide whether to stop the search after a solution was accepted. * * Returns `true` (and updates `result.status`) in two cases: * - the policy requests stopping at the first solution, or * - the configured maximum number of solutions has been reached. + * `@pre` `result.stats.solutions_found` already counts the solution that was + * just accepted.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@state_search_common.H` around lines 818 - 849, Add a short `@pre` to the stop_after_solution(Result&, const ExplorationPolicy&, const SearchLimits&) docblock stating that result.stats.solutions_found must already include the newly accepted solution before calling (so max_solutions is enforced correctly); mention the fields involved (result.stats.solutions_found and limits.max_solutions) and that the function may update result.status to SearchStatus::StoppedOnSolution or SearchStatus::LimitReached.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@state_search_common.H`:
- Around line 293-298: The comment incorrectly says there is "no compile-time
concept" for the Best_First restriction; remove that sentence and update the
paragraph so it references the existing SupportsBestFirst trait instead—i.e.,
delete the phrase about the lack of a compile-time concept and replace with a
short note that callers can rely on SupportsBestFirst to validate
engine/strategy compatibility where applicable while still ensuring the correct
runtime strategy is passed to engines like Aleph::Depth_First_Backtracking,
Aleph::IDA_Star_State_Search, Aleph::Negamax, and Aleph::Alpha_Beta.
- Around line 579-584: The Doxygen reference in the GoalPredicate note is
malformed: change the possessive `@ref Branch_And_Bound.H's` to a proper symbol
reference and wording (e.g., use `@ref` Branch_And_Bound::is_complete or write
"see Branch_And_Bound::is_complete()" outside the `@ref` token) so the doc points
to the engine's is_complete() symbol rather than a file-ish token; update the
sentence around `is_complete()`/`is_goal` accordingly to keep the same meaning.
---
Nitpick comments:
In `@state_search_common.H`:
- Around line 818-849: Add a short `@pre` to the stop_after_solution(Result&,
const ExplorationPolicy&, const SearchLimits&) docblock stating that
result.stats.solutions_found must already include the newly accepted solution
before calling (so max_solutions is enforced correctly); mention the fields
involved (result.stats.solutions_found and limits.max_solutions) and that the
function may update result.status to SearchStatus::StoppedOnSolution or
SearchStatus::LimitReached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d37b1ba8-b4ea-4634-9808-c008dd2f4159
📒 Files selected for processing (2)
Negamax.Hstate_search_common.H
🚧 Files skipped from review as they are similar to previous changes (1)
- Negamax.H
This commit refactors the search engines by introducing a `supports_best_first` static constexpr boolean. This allows for compile-time checks to determine if an engine supports the `Best_First` exploration strategy, preventing runtime `std::invalid_argument` exceptions. - `Backtracking.H`: Added `static constexpr bool supports_best_first = false;` - `State_Search_IDA_Star.H`: Added `static constexpr bool supports_best_first = false;` - `state_search_common.H`: Updated documentation for `ExplorationPolicy::Strategy::Best_First` to mention `IDA_Star_State_Search` and `Depth_First_Backtracking` do not support it, and introduced the `SupportsBestFirst` concept for compile-time validation. - `state_search_framework_test.cc`: Updated test names and assertions for IDA* with inadmissible heuristics.
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 `@state_search_common.H`:
- Around line 835-850: In stop_after_solution, the current early check of
policy.stop_at_first_solution can override the documented hard limit semantics
of limits.max_solutions; update the logic so the hard limit wins: when
limits.max_solutions != Search_Unlimited and result.stats.solutions_found >=
limits.max_solutions, increment result.stats.limit_hits and set result.status =
SearchStatus::LimitReached and return true before honoring
policy.stop_at_first_solution; keep the stop_at_first_solution branch only for
cases where the hard limit does not apply.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad285b40-3a26-48ae-a17c-5142de92f624
📒 Files selected for processing (4)
Backtracking.HState_Search_IDA_Star.HTests/state_search_framework_test.ccstate_search_common.H
🚧 Files skipped from review as they are similar to previous changes (1)
- Backtracking.H
Add new example files for backtracking, branch and bound, IDA*, and negamax. Improve stop_after_solution logic by prioritizing hard limits over soft stops. Add new test file for move ordering and a script to run all tests.
Refine comments for `AdversarialSearchResult` and `completed_iterations` to improve accuracy in `Negamax.H`. Update documentation in `state_search_common.H` to better reflect move ordering support and `Best_First` strategy limitations for various search engines.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Negamax.H (1)
319-323:⚠️ Potential issue | 🟠 MajorReject negative aspiration widths explicitly.
enabled()now treatshalf_window < 0as "disabled". Because the Negamax iterative-deepening entry points reject unsupported aspiration windows viaoptions.aspiration.enabled(), a caller can passhalf_window = -1and silently bypass that validation instead of getting an error.One straightforward fix
ah_invalid_argument_if(options.initial_depth > limits.max_depth) << "AdversarialIterativeDeepeningOptions::initial_depth exceeds SearchLimits::max_depth"; + ah_invalid_argument_if(options.aspiration.half_window < Score{}) + << "AspirationWindow::half_window must be non-negative"; ah_invalid_argument_if(options.aspiration.enabled()) << "Aspiration windows are only available through iterative_deepening_alpha_beta_search";Apply the same guard in both Negamax iterative-deepening overloads.
As per coding guidelines, "Validate external inputs before processing."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Negamax.H` around lines 319 - 323, The current enabled() check allows callers to bypass validation by passing a negative half_window; update both Negamax iterative-deepening entry points to explicitly reject negative aspiration widths by checking options.aspiration.half_window >= Score{} (in addition to options.aspiration.enabled()) and return/raise the same error path used for unsupported aspiration windows when half_window < Score{} so negative values are not silently treated as "disabled"; reference the aspiration fields/options via options.aspiration.half_window and the enabled() helper when adding this guard in each overload.
🧹 Nitpick comments (6)
run_all_tests.sh (1)
44-46: Consider using a subshell orpushd/popdfor directory changes.If
ctestfails and error handling evolves (e.g., a trap is added later), the working directory could remain changed. Using a subshell isolates the directory change and ensures the parent shell stays inPROJECT_ROOT.Suggested refactor
- cd "$build_dir" - ctest --output-on-failure || report_failure "Test execution failed for $mode mode." - cd "$PROJECT_ROOT" + (cd "$build_dir" && ctest --output-on-failure) || report_failure "Test execution failed for $mode mode."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run_all_tests.sh` around lines 44 - 46, The current sequence changes directory to "$build_dir", runs ctest, and then cd's back to "$PROJECT_ROOT", which can leak a changed working directory if ctest fails or future traps are added; update the block that uses cd "$build_dir" / ctest --output-on-failure || report_failure "Test execution failed for $mode mode." / cd "$PROJECT_ROOT" to isolate the directory change by either wrapping the cd and ctest call in a subshell (e.g., (cd "$build_dir" && ctest --output-on-failure) || report_failure ...) or by using pushd "$build_dir" and popd to ensure the parent shell always returns to "$PROJECT_ROOT"; reference the variables build_dir, PROJECT_ROOT and the ctest invocation when making this change.Examples/ida_star_no_solution_example.cc (1)
107-113: Inconsistentundo()implementation compared toInadmissibleDomain.This
undo()ignores themparameter and infers the parent purely from the current state, whileInadmissibleDomain::undo()(line 175) usesm.toto determine the parent. For consistency and clarity in example code, consider aligning both implementations.♻️ Suggested consistent implementation
void undo(State &s, const Move &m) const noexcept { - // Restore the previous node from the move (DAG: edges encode from→to). - (void) m; - // Parent tracking omitted for brevity; undo is required by the engine. - s.node = (s.node == 1) ? 0 : (s.node == 2) ? 1 : s.node; + // Reconstruct parent from move target (works for this linear DAG). + s.node = (m.to == 1) ? 0 : (m.to == 2) ? 1 : s.node; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/ida_star_no_solution_example.cc` around lines 107 - 113, The undo(State &s, const Move &m) const noexcept in this example inconsistently ignores the Move and infers parent from s.node; update it to mirror InadmissibleDomain::undo() by using m.to (the move's destination) to set s.node back to the parent instead of branching on s.node, ensuring the undo uses the provided Move parameter and behaves consistently with InadmissibleDomain::undo().Tests/move_ordering_test.cc (1)
230-242: Nice to have: assert full order, not only top-1, inKillerMovesFirst.Only checking Line 241 (
moves[0]) may miss regressions in secondary ordering (history/priority/ordinal behavior among non-killer moves).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/move_ordering_test.cc` around lines 230 - 242, Update the test "SortRankedMoves.KillerMovesFirst" to assert the full ordering of the sorted moves rather than only moves[0]; after calling sort_ranked_moves on the Array<RankedMove<int,int>> named moves, add expectations for moves[0], moves[1], and moves[2] (their .move or other relevant fields) to validate killer-first behavior and the subsequent history/priority/ordinal ordering produced by sort_ranked_moves, so regressions in secondary ordering are detected.Examples/branch_and_bound_df_vs_bf_example.cc (2)
47-49: Nice to have: align docs with behavior (“verifies” vs currently only prints).Lines 47-49 say the example verifies equality, but
run_strategyreturnsvoidand no comparison is performed. Either compare returned best values or reword the comment.Also applies to: 193-216
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/branch_and_bound_df_vs_bf_example.cc` around lines 47 - 49, The comment claims the example "verifies that they agree on the optimum value" but the implementation of run_strategy returns void and no comparison is done; update code to actually return and compare the best values (e.g., change run_strategy to return the best objective found and call it for both strategies in main, then assert or check equality and print a verification message) or change the comment text to say it only prints side-by-side results; target the run_strategy function and the main/printing block (lines around where both strategies are invoked and results are printed) when making the change.
93-97: Should fix: enforcebound()precondition instead of relying on caller discipline.
KnapsackDomain::boundassumes remaining items are in non-increasing value density order. Add a constructor-time check (or internal ordering) so future datasets cannot silently break admissibility.As per coding guidelines: "Validate input preconditions (e.g., non-negative weights for Dijkstra)" and "Fail fast with clear error messages."
Also applies to: 116-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/branch_and_bound_df_vs_bf_example.cc` around lines 93 - 97, KnapsackDomain currently assumes items_ are sorted by non-increasing value density for bound() to be admissible; enforce this in the constructor(s) by either validating and failing fast (throw with a clear message) when items_ are not in non-increasing value density order or by internally sorting items_ by value density there; update the explicit KnapsackDomain(const Array<Item>& items, double capacity) constructor and the other constructor overload(s) that initialize items_ (e.g., the ones around the same class) to perform this check or sort so bound() no longer relies on caller discipline.Examples/negamax_simple_example.cc (1)
170-173: Consider demonstratinghas_principal_variation()before callingfirst_move().This root state always has a move, but guarding
first_move()would make the sample resilient to small tweaks like a terminal initial board or a shallow limit, and it would showcase the new helper at the same time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Examples/negamax_simple_example.cc` around lines 170 - 173, The sample should check result.has_principal_variation() before calling result.first_move() to avoid assuming a principal variation exists; update the printing logic around the result object to call result.has_principal_variation() and, if true, print the cell from result.first_move().cell, otherwise print a clear fallback message (e.g., "no principal variation" or "no legal moves") so the example remains correct for terminal or shallow-limit root states.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Examples/backtracking_cyclic_graph_example.cc`:
- Line 136: Update the inline comment that currently reads "Depth-aware visited
map: key=node id, value=depth at first visit" to accurately reflect that the map
stores the minimum depth seen and is updated if a shorter path is later
discovered; reference the visited/visited-depth map used by the backtracking
engine (the map that holds node id → depth) and change the wording to something
like "Depth-aware visited map: key=node id, value=minimum depth seen (updated if
a shorter path is found)" so readers understand the stored depth can be revised.
In `@Examples/branch_and_bound_df_vs_bf_example.cc`:
- Around line 224-231: The items array used for the branch-and-bound fractional
bound is not sorted by value/weight (density), causing the optimistic bound in
bound() to be underestimated and valid branches to be pruned; fix by reordering
the const Array<Item> items so that entries are sorted descending by
value/weight (i.e., ensure "tablet" (13/4 = 3.25) comes before "laptop" (16/5 =
3.2)), or alternatively compute and sort the items by density before using them
in the branch-and-bound logic (update the items initializer referenced in
Examples::branch_and_bound_df_vs_bf_example and ensure any code that assumes
pre-sorted input uses the sorted array).
In `@Negamax.H`:
- Around line 358-361: The docstring for has_iterations() is misleading: the
loop in run_iterative_deepening() appends the last iteration before
limit_reached(), so has_iterations() (which returns not iterations.is_empty())
means "at least one iteration result was recorded", not "at least one iteration
completed"; update the documentation for has_iterations() to reflect this
behavior and add `@pre/`@post as appropriate (or mention completed_iterations vs
recorded iterations) and ensure the comments reference the iterations container
and the completed_iterations semantic so readers see the distinction.
- Around line 1162-1175: The code assumes domain_.apply/state mutations are
atomic and domain_.undo won't throw; instead make that contract explicit by
either (A) changing the domain API to provide a strong-exception-safe
apply/rollback pair (e.g., domain_.begin_apply/state token or domain_.try_apply
that returns a handle) and use that handle to call domain_.undo on scope exit,
or (B) introduce an RAII guard in search_node that immediately marks “applied”
before any partial mutation is possible only if domain provides a no-throw
guarantee, otherwise call a new no-throw wrapper like domain_.safe_apply(state,
move) which either completes or leaves state unchanged; update calls around
domain_.apply/domain_.undo (the try/catch using applied, search_node, and the
NodeEvaluation<Move, Score> child creation) to use the new API/guard and add
tests covering throw-after-mutate and undo-throws scenarios to make the
exception-safety contract explicit.
In `@run_all_tests.sh`:
- Line 40: The build invocation uses unquoted command substitution $(nproc)
which can cause word splitting; update the cmake --build invocation so the
parallel jobs argument uses a quoted substitution (i.e., change the -j$(nproc)
usage to use "$(nproc)") while keeping the same error handling via
report_failure and the existing variables build_dir and mode.
In `@Tests/move_ordering_test.cc`:
- Around line 41-210: The tests lack large-input edge-case coverage for
Killer_Move_Table and History_Heuristic_Table; add new unit tests that (1)
exercise many depths (e.g., thousands) by calling
Killer_Move_Table::record(depth, move) across a wide range and assert is_killer
for recorded depths and not-recorded depths, verify eviction behavior holds
under many inserts (record more than two entries per depth) and that clear()
resets all; and (2) exercise many distinct keys for History_Heuristic_Table by
calling History_Heuristic_Table::record(key, bonus) for a large set of keys and
accumulating bonuses, asserting score(key) matches expected sums and that
clear() resets them; reuse existing test patterns (record, is_killer, score,
clear) and include a stress test variant for Null_History_Heuristic_Table and
non-comparable Killer_Move_Table to confirm they remain no-ops under large
inputs.
- Around line 214-315: Add deterministic randomized stress and performance
regression tests for sort_ranked_moves: create new tests (e.g.,
SortRankedMoves.RandomizedStress and SortRankedMoves.PerformanceRegression) that
generate large arrays of RankedMove<T,U> using a fixed RNG seed, vary priority,
ordinal, killer flag and history values, and exercise combinations of the two
flags (killer_first, use_history). For correctness: verify post-sort invariants
for sort_ranked_moves (killer entries precede non-killers when killer_first is
true, ties broken by history when use_history is true, otherwise by ordinal, and
otherwise by the provided priority comparator), and cross-check by comparing to
a reference stable sort implementation using the same comparator logic. For
performance: run the function on large inputs (e.g., N in the thousands) and
assert the elapsed time stays below a defined threshold to catch regressions.
Ensure tests are deterministic by seeding the RNG and include single-element and
empty-array cases in the randomized suite.
- Around line 80-89: The test DuplicateRecordIsNoOp doesn't exercise the
duplicate-primary no-op because the third call table.record(0, 10) reintroduces
the secondary value (rotation) instead of repeating the current primary; change
the third record to repeat the current primary value so the no-op path is hit
(i.e., in the TEST KillerMoveTable DuplicateRecordIsNoOp, after table.record(0,
10); table.record(0, 20); replace the final table.record(0, 10); with
table.record(0, 20); and keep the EXPECT_TRUE checks using table.is_killer).
---
Outside diff comments:
In `@Negamax.H`:
- Around line 319-323: The current enabled() check allows callers to bypass
validation by passing a negative half_window; update both Negamax
iterative-deepening entry points to explicitly reject negative aspiration widths
by checking options.aspiration.half_window >= Score{} (in addition to
options.aspiration.enabled()) and return/raise the same error path used for
unsupported aspiration windows when half_window < Score{} so negative values are
not silently treated as "disabled"; reference the aspiration fields/options via
options.aspiration.half_window and the enabled() helper when adding this guard
in each overload.
---
Nitpick comments:
In `@Examples/branch_and_bound_df_vs_bf_example.cc`:
- Around line 47-49: The comment claims the example "verifies that they agree on
the optimum value" but the implementation of run_strategy returns void and no
comparison is done; update code to actually return and compare the best values
(e.g., change run_strategy to return the best objective found and call it for
both strategies in main, then assert or check equality and print a verification
message) or change the comment text to say it only prints side-by-side results;
target the run_strategy function and the main/printing block (lines around where
both strategies are invoked and results are printed) when making the change.
- Around line 93-97: KnapsackDomain currently assumes items_ are sorted by
non-increasing value density for bound() to be admissible; enforce this in the
constructor(s) by either validating and failing fast (throw with a clear
message) when items_ are not in non-increasing value density order or by
internally sorting items_ by value density there; update the explicit
KnapsackDomain(const Array<Item>& items, double capacity) constructor and the
other constructor overload(s) that initialize items_ (e.g., the ones around the
same class) to perform this check or sort so bound() no longer relies on caller
discipline.
In `@Examples/ida_star_no_solution_example.cc`:
- Around line 107-113: The undo(State &s, const Move &m) const noexcept in this
example inconsistently ignores the Move and infers parent from s.node; update it
to mirror InadmissibleDomain::undo() by using m.to (the move's destination) to
set s.node back to the parent instead of branching on s.node, ensuring the undo
uses the provided Move parameter and behaves consistently with
InadmissibleDomain::undo().
In `@Examples/negamax_simple_example.cc`:
- Around line 170-173: The sample should check result.has_principal_variation()
before calling result.first_move() to avoid assuming a principal variation
exists; update the printing logic around the result object to call
result.has_principal_variation() and, if true, print the cell from
result.first_move().cell, otherwise print a clear fallback message (e.g., "no
principal variation" or "no legal moves") so the example remains correct for
terminal or shallow-limit root states.
In `@run_all_tests.sh`:
- Around line 44-46: The current sequence changes directory to "$build_dir",
runs ctest, and then cd's back to "$PROJECT_ROOT", which can leak a changed
working directory if ctest fails or future traps are added; update the block
that uses cd "$build_dir" / ctest --output-on-failure || report_failure "Test
execution failed for $mode mode." / cd "$PROJECT_ROOT" to isolate the directory
change by either wrapping the cd and ctest call in a subshell (e.g., (cd
"$build_dir" && ctest --output-on-failure) || report_failure ...) or by using
pushd "$build_dir" and popd to ensure the parent shell always returns to
"$PROJECT_ROOT"; reference the variables build_dir, PROJECT_ROOT and the ctest
invocation when making this change.
In `@Tests/move_ordering_test.cc`:
- Around line 230-242: Update the test "SortRankedMoves.KillerMovesFirst" to
assert the full ordering of the sorted moves rather than only moves[0]; after
calling sort_ranked_moves on the Array<RankedMove<int,int>> named moves, add
expectations for moves[0], moves[1], and moves[2] (their .move or other relevant
fields) to validate killer-first behavior and the subsequent
history/priority/ordinal ordering produced by sort_ranked_moves, so regressions
in secondary ordering are detected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: af4aca7b-3c1d-4089-86fd-1cb0c655b031
📒 Files selected for processing (8)
Examples/backtracking_cyclic_graph_example.ccExamples/branch_and_bound_df_vs_bf_example.ccExamples/ida_star_no_solution_example.ccExamples/negamax_simple_example.ccNegamax.HTests/move_ordering_test.ccrun_all_tests.shstate_search_common.H
🚧 Files skipped from review as they are similar to previous changes (1)
- state_search_common.H
Refactor move ordering tests, adding comprehensive coverage for `Killer_Move_Table` and `History_Heuristic_Table` with large input and randomized stress tests. Update `Alpha_Beta.H` and `Negamax.H` to add validation for aspiration window settings and refine iterative deepening options. Clarify documentation for `has_iterations()` in `Negamax.H` and update example comments.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
lrleon
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
run_all_tests.sh (1)
40-40:⚠️ Potential issue | 🟡 MinorQuote and harden CPU-count expansion on Line 40.
-j$(nproc)is still unquoted and can fail on systems withoutnproc. Compute jobs once with fallback, then quote it.Proposed fix
run_stage() { local mode=$1 local build_dir=$2 + local jobs + jobs="$(command -v nproc >/dev/null 2>&1 && nproc || getconf _NPROCESSORS_ONLN || echo 1)" @@ - cmake --build "$build_dir" -j$(nproc) || report_failure "Compilation failed for $mode mode." + cmake --build "$build_dir" -j"$jobs" || report_failure "Compilation failed for $mode mode."#!/bin/bash # Verify current parallel-build argument handling in run_all_tests.sh rg -n -C2 --type=sh 'cmake --build .* -j' run_all_tests.sh rg -n --type=sh '\bnproc\b|getconf _NPROCESSORS_ONLN|hw\.ncpu' run_all_tests.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run_all_tests.sh` at line 40, The cmake invocation uses an unquoted -j$(nproc) which can fail on systems without nproc; compute a jobs variable once (e.g., JOBS or NUM_JOBS) with a fallback like: JOBS="$(command -v nproc >/dev/null 2>&1 && nproc || getconf _NPROCESSORS_ONLN 2>/dev/null || echo 1)" and then use it quoted in the build command (replace cmake --build "$build_dir" -j$(nproc) ... with cmake --build "$build_dir" -j"$JOBS" ...) so expansion is robust across environments.Tests/move_ordering_test.cc (1)
412-473:⚠️ Potential issue | 🟠 MajorShould fix: add a regression guard for
sort_ranked_moveshot-path performance.The new random/large-input cases improve correctness coverage, but this still does not pin sort cost on a fixed large corpus. A coarse large-
Nbudget or a comparison-count proxy would catch accidental complexity regressions here. Based on learnings: "Applies to Tests/**/*_test.cc : Include performance regression tests for critical algorithms."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/move_ordering_test.cc` around lines 412 - 473, Add a performance regression guard around the sort_ranked_moves hot path in the SortRankedMoves.RandomizedStress test: instrument the comparator used by sort_ranked_moves (and/or the sort implementation) with a lightweight comparison counter, run a coarse large-N case (e.g., N increased from 1000 to a bigger check) or a separate sub-test, and assert the total comparison count stays below a chosen budget (e.g., C * N * log2(N) for a small constant C) to catch accidental complexity regressions; modify the test code in Tests/move_ordering_test.cc (inside the RandomizedStress / TEST SortRankedMoves block) to wrap the provided lambda comparator with a counter, expose the count after sort_ranked_moves returns, and add an ASSERT/EXPECT that enforces the comparison-count budget.
🧹 Nitpick comments (2)
run_all_tests.sh (1)
1-64: Structured review buckets for this file
Must fix
- None beyond the Line 40 issue already flagged.
Should fix
- Since this script changes test orchestration behavior (Debug + Release, fail-fast), add CI coverage that invokes
run_all_tests.sh, or explicitly document why script-level tests do not apply.Nice to have
- Replace
cd "$build_dir"/cd "$PROJECT_ROOT"withctest --test-dir "$build_dir" --output-on-failureto avoid directory-state coupling.As per coding guidelines, "Provide feedback in these sections: 'Must fix', 'Should fix', 'Nice to have'." and "If the PR changes behavior, require tests or explain why they don't apply."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@run_all_tests.sh` around lines 1 - 64, The run_stage function currently changes directories with cd "$build_dir" / cd "$PROJECT_ROOT" before invoking ctest which couples script state; replace that sequence by calling ctest with an explicit test directory (ctest --test-dir "$build_dir" --output-on-failure) inside run_stage and remove the cd commands (refer to run_stage, ctest, BUILD_DIR_DEBUG/BUILD_DIR_RELEASE and report_failure to locate the code), and also add CI coverage that invokes run_all_tests.sh (or add a short note in project docs/README explaining why invoking script-level tests is omitted) so the changed orchestration (Debug+Release fail-fast) is exercised or justified.Tests/move_ordering_test.cc (1)
416-439: Nice to have: reuse one seeded corpus across all four flag combinations.Because
rngkeeps advancing through the nested loops, each(killer_first, use_history)mode sorts a different input set. Re-seeding per mode, or building one baseline vector and copying it intomoves, makes failures much easier to compare across configurations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/move_ordering_test.cc` around lines 416 - 439, The test currently advances rng across the nested loops so each (killer_first, use_history) case gets different inputs; to make failures comparable, seed and build a single baseline corpus once (using N, SEED, rand_int/rand_uint/rand_bool or by using the existing rng to fill a vector of RankedMove<int,int> called baseline_moves) before the for-loops, then inside each combination replace the creation of moves with a copy from baseline_moves (e.g., moves = baseline_moves or moves.append copies) or alternatively re-seed rng with SEED at the start of each iteration; update the code around moves, rng, and sort_ranked_moves to use this shared, deterministic input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Tests/move_ordering_test.cc`:
- Around line 448-470: The test currently skips assertions for mixed
killer/non-killer pairs because of the "if (a.killer == b.killer)" guard and
also fails to verify the priority/ordinal fallback when use_history is true but
history scores tie; update the logic in move_ordering_test.cc to (1) only apply
the killer vs non-killer special-case when prefer_killer/killer_first semantics
require it (i.e., check killer_first/prefer_killer rather than gating all checks
on a.killer==b.killer), (2) for mixed killer/non-killer pairs when killer_first
is false still run the history/priority/ordinal comparisons, and (3) when
use_history is true and a.history_score == b.history_score fall back to checking
priority (a.priority >= b.priority) and then ordinal tiebreak (a.ordinal <=
b.ordinal) so those branches get validated; locate and modify the block
referencing killer_first, a.killer/b.killer, use_history, a.history_score,
a.priority and a.ordinal to implement this behavior.
---
Duplicate comments:
In `@run_all_tests.sh`:
- Line 40: The cmake invocation uses an unquoted -j$(nproc) which can fail on
systems without nproc; compute a jobs variable once (e.g., JOBS or NUM_JOBS)
with a fallback like: JOBS="$(command -v nproc >/dev/null 2>&1 && nproc ||
getconf _NPROCESSORS_ONLN 2>/dev/null || echo 1)" and then use it quoted in the
build command (replace cmake --build "$build_dir" -j$(nproc) ... with cmake
--build "$build_dir" -j"$JOBS" ...) so expansion is robust across environments.
In `@Tests/move_ordering_test.cc`:
- Around line 412-473: Add a performance regression guard around the
sort_ranked_moves hot path in the SortRankedMoves.RandomizedStress test:
instrument the comparator used by sort_ranked_moves (and/or the sort
implementation) with a lightweight comparison counter, run a coarse large-N case
(e.g., N increased from 1000 to a bigger check) or a separate sub-test, and
assert the total comparison count stays below a chosen budget (e.g., C * N *
log2(N) for a small constant C) to catch accidental complexity regressions;
modify the test code in Tests/move_ordering_test.cc (inside the RandomizedStress
/ TEST SortRankedMoves block) to wrap the provided lambda comparator with a
counter, expose the count after sort_ranked_moves returns, and add an
ASSERT/EXPECT that enforces the comparison-count budget.
---
Nitpick comments:
In `@run_all_tests.sh`:
- Around line 1-64: The run_stage function currently changes directories with cd
"$build_dir" / cd "$PROJECT_ROOT" before invoking ctest which couples script
state; replace that sequence by calling ctest with an explicit test directory
(ctest --test-dir "$build_dir" --output-on-failure) inside run_stage and remove
the cd commands (refer to run_stage, ctest, BUILD_DIR_DEBUG/BUILD_DIR_RELEASE
and report_failure to locate the code), and also add CI coverage that invokes
run_all_tests.sh (or add a short note in project docs/README explaining why
invoking script-level tests is omitted) so the changed orchestration
(Debug+Release fail-fast) is exercised or justified.
In `@Tests/move_ordering_test.cc`:
- Around line 416-439: The test currently advances rng across the nested loops
so each (killer_first, use_history) case gets different inputs; to make failures
comparable, seed and build a single baseline corpus once (using N, SEED,
rand_int/rand_uint/rand_bool or by using the existing rng to fill a vector of
RankedMove<int,int> called baseline_moves) before the for-loops, then inside
each combination replace the creation of moves with a copy from baseline_moves
(e.g., moves = baseline_moves or moves.append copies) or alternatively re-seed
rng with SEED at the start of each iteration; update the code around moves, rng,
and sort_ranked_moves to use this shared, deterministic input.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da430337-c79e-4190-a0d0-87baef8b06d9
📒 Files selected for processing (9)
Alpha_Beta.HExamples/backtracking_cyclic_graph_example.ccExamples/branch_and_bound_df_vs_bf_example.ccExamples/ida_star_no_solution_example.ccExamples/negamax_simple_example.ccNegamax.HTests/move_ordering_test.ccrun_all_tests.shstate_search_common.H
✅ Files skipped from review due to trivial changes (1)
- Examples/backtracking_cyclic_graph_example.cc
🚧 Files skipped from review as they are similar to previous changes (3)
- Examples/negamax_simple_example.cc
- Examples/branch_and_bound_df_vs_bf_example.cc
- Alpha_Beta.H
- Increases sample size from 1000 to 2000 in random stress test - Adds O(N log N) complexity check with 20*N*log2(N) limit - Simplifies verification logic by eliminating redundant condition - Includes benchmark comparisons to detect performance regressions - Update test script to use ctest --test-dir and detect available cores - Document run_all_tests.sh script in README for local verification
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
Bug Fixes
Refactor
Documentation
Tests