feat(mab): integrate Lin-Kernighan with Multi-Armed Bandit scheduler#28
feat(mab): integrate Lin-Kernighan with Multi-Armed Bandit scheduler#28
Conversation
…ector Implements Task 6.2: Integration of Lin-Kernighan with Multi-Armed Bandit scheduler. Core Changes: - Add LocalSearchOperator concept to define interface for local search algorithms - Implement AdaptiveLocalSearchSelector class template for adaptive local search selection - Add UCBLocalSearchSelector and ThompsonLocalSearchSelector type aliases - Include Lin-Kernighan header in main evolab.hpp Features: - Performance tracking with execution time measurement (chrono-based) - Improvement rate tracking via OperatorStats - Unified interface matching AdaptiveOperatorSelector pattern - Support for both UCB and Thompson Sampling schedulers Testing: - Add comprehensive test suite (test_mab_local_search.cpp) with 21 tests - Add assert_le and assert_gt(double) methods to test_helper.hpp - All tests pass (TDD GREEN phase) The AdaptiveLocalSearchSelector enables researchers to automatically select the best-performing local search operator based on historical performance, allowing for hybrid memetic algorithms that adapt during evolution. Related: Task 6.1 (Lin-Kernighan implementation) Ref: .kiro/specs/evolab/tasks.md
Task 6.2 (Lin-Kernighan with Multi-Armed Bandit scheduler) is complete. Completed: - LocalSearchOperator concept - AdaptiveLocalSearchSelector implementation - Performance tracking (execution time, improvement rate) - Comprehensive test coverage (21 tests) Deferred for future work: - Configuration system integration (needs runtime operator selection) - Hybrid Memetic GA configuration templates The core MAB local search integration is functional and tested.
Summary of ChangesHello @lv416e, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds AdaptiveLocalSearchSelector (UCB/Thompson) and LocalSearchOperator support with chrono-based timing and expanded OperatorStats; exposes Lin–Kernighan via public include; introduces thread-local RNG accessor; expands tests, test helpers, and CTest entry for MAB local-search integration. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GA as GeneticAlgorithm
participant ALS as AdaptiveLocalSearchSelector
participant SCH as Scheduler (UCB/Thompson)
participant OP as LocalSearchOperator
participant RNG as ThreadRNG
participant T as Timer
rect rgb(230,245,255)
GA->>ALS: apply_local_search(problem, genome, RNG)
end
rect rgb(255,250,230)
ALS->>T: start timing
ALS->>SCH: select_operator(operator_stats)
SCH-->>ALS: selected_index
end
rect rgb(240,255,240)
ALS->>OP: invoke selected operator(problem, genome, RNG)
OP-->>ALS: new_fitness
ALS->>T: stop timing
ALS->>ALS: compute improvement & record execution time
ALS->>SCH: report_reward(improvement)
ALS-->>GA: return new_fitness
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
… corruption
Critical fix: When selected operator index is out of bounds,
throw std::out_of_range instead of returning default Fitness{0.0}.
This prevents incorrect reward calculation that could corrupt
the MAB scheduler's learning process.
- Add <stdexcept> header for exception support
- Replace silent failure with explicit exception
- Provide clear error message about configuration mismatch
Addresses Gemini code review critical issue
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Performance and correctness improvements: - Pass operator by value and move into lambda capture - Mark lambda as mutable for non-const improve() methods - Avoids unnecessary copies for large operator objects - Enables move-only operator types in the future Follows modern C++ best practices for type-erased storage Addresses Gemini code review medium priority issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Reduce code duplication: - Create TestTSPInstance helper struct to encapsulate TSP instance and tour initialization - Used in 5 test functions: test_apply_local_search, test_performance_tracking, test_execution_time_tracking, test_improvement_rate_tracking, test_thompson_sampling_integration - Improves maintainability and readability Addresses Gemini code review medium priority issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Critical fixes to prevent crashes when no operators are configured: 1. AdaptiveLocalSearchSelector::apply_local_search: - Check if operators_ is empty before calling scheduler - Throw logic_error with clear message 2. UCBScheduler::select_operator: - Check if stats_ is empty before selection - Prevents out-of-bounds access on empty vector 3. ThompsonSamplingScheduler::select_operator: - Check if distributions_ is empty before sampling - Prevents invalid max_element and distance calls These checks ensure fail-fast behavior with clear error messages instead of undefined behavior or crashes. Addresses Gemini and CodeRabbit critical review issues 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add bounds check in AdaptiveLocalSearchSelector::add_operator: - Throw logic_error if operators_.size() >= num_operators - Prevents silent failures where extra operators are never selected - Provides clear error message about capacity mismatch This ensures API misuse is caught early rather than leading to confusing runtime behavior. Addresses Gemini code review medium priority issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Simplify improvement rate tracking test: - Remove redundant calculation: success_count / selection_count - Use pre-computed stat.success_rate from OperatorStats - Update assertion messages to "Success rate" for accuracy The OperatorStats struct already maintains success_rate, so recalculating it in tests is unnecessary duplication. Addresses Gemini code review medium priority issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Docstrings generation was requested by @lv416e. * #28 (comment) The following files were modified: * `include/evolab/schedulers/mab.hpp` * `tests/test_helper.hpp` * `tests/test_mab_local_search.cpp`
Consolidate LocalSearchOperator concept definitions: - Remove duplicate concept from mab.hpp (was lines 24-28) - Include core/concepts.hpp for canonical definition - Update template constraint to use core::LocalSearchOperator - Remove unnecessary mutable keyword from lambda capture - Update test to use core::LocalSearchOperator The core/concepts.hpp definition requires const-qualified improve() methods, which all operators (LinKernighan, TwoOpt, Random2Opt) already satisfy. Benefits: - Single source of truth for core abstractions - Eliminates confusion from duplicate definitions - Ensures consistency across the codebase - Simplifies maintenance Addresses Gemini code review high priority issue 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add tracking state validation to prevent silent failures:
1. AdaptiveLocalSearchSelector::apply_local_search:
- Check if tracking_improvement_ is true before starting
- Throw logic_error if apply_local_search is called again
before report_fitness_improvement
2. AdaptiveOperatorSelector::apply_crossover:
- Same check to prevent consecutive calls without reporting
- Ensures scheduler learning is not corrupted by lost tracking
Problem scenario:
- User calls apply_local_search() twice consecutively
- First call's tracking is overwritten by second call
- When report_fitness_improvement() is finally called,
only the second operation's reward is recorded
- Scheduler learns from incorrect data
Solution:
- Fail fast with clear error message
- Forces correct API usage pattern:
apply_local_search -> report_fitness_improvement -> repeat
Benefits:
- Prevents silent bugs in scheduler learning
- Makes API contract explicit and enforceable
- Easier debugging with clear error messages
Addresses Gemini code review medium priority issue
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Address code review feedback from Gemini and CodeRabbit: 1. Add missing <limits> include for std::numeric_limits - Prevents fragile transitive dependency issues 2. Add defensive checks to apply_crossover - Add empty() check to match apply_local_search parity - Replace silent fallback with std::out_of_range exception - Improves API contract enforcement 3. Fix tracking_improvement_ timing for exception safety - Move assignment after successful operator execution - Prevents sticky state when exceptions occur - Applies to both apply_crossover and apply_local_search 4. Replace high_resolution_clock with steady_clock - Guarantees monotonic time measurement - Prevents time anomalies from clock adjustments All tests pass (21/21). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Move timing start point after OOB validation in apply_local_search. This provides more accurate operator performance metrics by excluding the validation overhead from execution time measurement. Address CodeRabbit nitpick feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add TODO comment documenting Gemini's suggestion to extract common functionality into a base class. While the suggestion is valid from a DRY perspective, we defer this refactoring for the following reasons: 1. YAGNI principle: Only two selector types exist currently 2. Complexity trade-off: Template+inheritance would increase complexity 3. Clear separation: Each selector has distinct responsibilities 4. Maintainability: Current duplication is manageable and understandable The TODO serves as a reminder to revisit this decision if a third selector type is needed, at which point the refactoring would provide clear value. Address Gemini medium-priority suggestion with reasoned deferral. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Update the TODO comment to reflect that the code duplication between AdaptiveOperatorSelector and AdaptiveLocalSearchSelector has reached ~110 lines, exceeding the original 100-line threshold. The comment now acknowledges this and prioritizes refactoring before adding a third selector type to prevent further code multiplication. This addresses code review feedback noting that the duplication threshold may have already been met. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add reference to GitHub Issue #32 in the TODO comment about selector class refactoring. This provides traceability for the planned work to eliminate ~110 lines of code duplication between AdaptiveOperatorSelector and AdaptiveLocalSearchSelector. The issue tracks the medium-high priority refactoring that should be completed before adding a third selector type. Addresses code review feedback recommending follow-up refactoring. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Improve error diagnostics in AdaptiveOperatorSelector and AdaptiveLocalSearchSelector: - add_operator(): Include both maximum allowed and current operator counts to help identify configuration mismatches between constructor parameter and actual operator additions - apply_crossover()/apply_local_search(): Include the actual selected operator index in out-of-bounds error to pinpoint scheduler selection issues These changes make debugging configuration errors significantly easier by providing concrete values instead of generic messages. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add inline comment explaining why the 64-bit clock_seed is split into two 32-bit values via right shift (>> 32). This improves code comprehension by making the entropy distribution strategy explicit. The split ensures both the lower and upper 32 bits of the timestamp contribute independently to the seed sequence, maximizing entropy utilization for thread-local RNG initialization. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Address research integrity and correctness issues identified in critical analysis. These changes are essential for safe and correct use of MAB scheduler in research contexts. Changes: 1. Thread-safety warnings (CRITICAL - Research Integrity): - Add explicit warnings to AdaptiveOperatorSelector class docs - Add explicit warnings to AdaptiveLocalSearchSelector class docs - Document race condition risks in parallel GAs - Specify requirement: one selector instance per thread - Impact: Prevents incorrect research results from data races 2. Minimization assumption documentation (CRITICAL - Correctness): - Document report_fitness_change() assumes minimization problems - Add @warning tags for maximization problem usage - Provide code examples for both problem types - Applied to both AdaptiveOperatorSelector and AdaptiveLocalSearchSelector - Impact: Prevents incorrect operator learning from wrong improvement calculation 3. Empirical validation roadmap (HIGH PRIORITY - Research Quality): - Add comprehensive TODO comment documenting required validation - Detail 6 categories of validation experiments needed - Specify acceptance criteria and success metrics - Estimate effort (2-3 days) and priority (HIGH, blocks v1.0) - Reference GitHub Issue #33 for tracking - Impact: Clarifies path to research-grade quality 4. PR description updates: - Remove inaccurate TDD claim (tests were added after implementation) - Add "Known Limitations" section with thread-safety and minimization constraints - Add detailed "Future Work" section with Issue #33 link - Update checklist to reflect documentation improvements - Impact: Sets accurate expectations for feature maturity Rationale: After critical thinking + ultrathink analysis, these issues were identified as blocking concerns for a research framework: - Thread-safety: Silent data corruption in parallel GAs → wrong results - Minimization assumption: Breaks for maximization problems → wrong learning - Validation gap: No evidence feature works as claimed → not research-grade These 30 minutes of documentation work address research ethics and correctness issues that could lead to incorrect published results. Related: Issue #33 (Empirical Validation) Related: PR #28 (MAB Implementation) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extract ~60 lines of detailed validation plan from inline comment to Issue #33 for better maintainability and task tracking. Code comment now contains brief reference to issue with link. Addresses CodeRabbit review feedback on code readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ction Add <sstream> header include to support stringstream-based error message construction, replacing inefficient string concatenation with + operator. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…rator error Replace inefficient string concatenation with std::stringstream for constructing error messages in AdaptiveOperatorSelector::add_operator(). This avoids multiple temporary string allocations and improves code readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…rossover error Replace inefficient string concatenation with std::stringstream for constructing error messages in AdaptiveOperatorSelector::apply_crossover(). This avoids multiple temporary string allocations and improves code readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…operator error Replace inefficient string concatenation with std::stringstream for constructing error messages in AdaptiveLocalSearchSelector::add_operator(). This avoids multiple temporary string allocations and improves code readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…y_local_search error Replace inefficient string concatenation with std::stringstream for constructing error messages in AdaptiveLocalSearchSelector::apply_local_search(). This avoids multiple temporary string allocations and improves code readability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add <thread> header include to support thread ID incorporation in random number generator seeding, enabling more robust seed generation in parallel execution scenarios. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…tion Enhance thread-local random number generator seeding by incorporating thread ID hash into seed sequence. This provides an additional entropy source that is guaranteed to be unique per thread, significantly reducing the risk of seed collisions in parallel execution scenarios with rapid, concurrent thread creation. This improvement is particularly important for parallel genetic algorithms (e.g., Island Model) where multiple threads may be spawned simultaneously, ensuring better reproducibility and statistical independence across threads. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…d_operator Apply perfect forwarding to the operator parameter in add_operator() method of AdaptiveOperatorSelector. This optimization avoids unnecessary copies when l-value operators are passed and correctly handles move semantics for both l-values and r-values, improving performance and adhering to modern C++ best practices. Changes: - Changed parameter from `OpType op` to `OpType&& op` - Updated lambda capture from `std::move(op)` to `std::forward<OpType>(op)` 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…:add_operator Apply perfect forwarding to the operator parameter in add_operator() method of AdaptiveLocalSearchSelector. This optimization provides the same benefits as the corresponding change in AdaptiveOperatorSelector: avoiding unnecessary copies for l-value operators and correctly handling move semantics for both l-values and r-values. Changes: - Changed parameter from `OpType op` to `OpType&& op` - Updated lambda capture from `std::move(op)` to `std::forward<OpType>(op)` This ensures both selector classes follow modern C++ best practices and maintain consistent implementation patterns. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add optional tolerance parameters to double comparison functions to improve test reliability and prevent flaky tests from floating-point precision issues. Changes: - assert_ge(double): Add eps parameter (default: 0.0) - assert_gt(double): Add eps parameter (default: 1e-9) with diff output - assert_le(double): Add tol parameter (default: 1e-9) - assert_lt(double): Add tol parameter (default: 1e-9) with diff output Based on CodeRabbit review feedback to make comparisons more robust. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Implements Task 6.2: Integration of Lin-Kernighan local search with Multi-Armed Bandit (MAB) scheduler for adaptive local search operator selection in memetic algorithms.
Changes
Core Implementation
LocalSearchOperator concept: Type-safe interface for local search algorithms
improve(problem, genome, rng) -> FitnessmethodAdaptiveLocalSearchSelector: Template class for adaptive local search selection
UCBLocalSearchSelector<Problem>,ThompsonLocalSearchSelector<Problem>Performance Tracking
std::chrono::steady_clockOperatorStatsinfrastructureTesting
tests/test_mab_local_search.cpp(21 tests, all passing)assert_leandassert_gt(double)methodsIntegration
include/evolab/local_search/lk.hppto mainevolab.hppheaderUsage Example
Known Limitations (Pre-v1.0)
Thread Safety
AdaptiveOperatorSelectorandAdaptiveLocalSearchSelectormaintain mutable stateMinimization Problems Only
report_fitness_change(old_fitness, new_fitness)assumes minimization (lower = better)old_fitness - new_fitnessreport_fitness_improvement()directly:Empirical Validation Pending
Future Work
Before v1.0 Release (Blocking)
Post-v1.0 (Enhancement)
Selector refactoring (Issue MAB Scheduler: Refactor Selector Classes #32) - MEDIUM PRIORITY
Configuration system integration - MEDIUM PRIORITY
scheduler.operators = ["LK", "2-opt"]in config filesMaximization support - LOW PRIORITY
Testing Results
Related Work
Checklist
Generated with Claude Code
Co-Authored-By: Claude noreply@anthropic.com
Summary by CodeRabbit