refactor(mab): eliminate code duplication using policy-based design#34
refactor(mab): eliminate code duplication using policy-based design#34
Conversation
Introduce CrossoverOperatorTraits and LocalSearchOperatorTraits to enable policy-based design for adaptive operator selection. These traits encapsulate type differences between crossover and local search operators, providing: - Type definitions (ResultType, OperatorFn) - Concept-constrained wrap_operator() methods - Type-safe operator wrapping with C++20 concepts This is the foundation for eliminating ~110 lines of duplicated code between AdaptiveOperatorSelector and AdaptiveLocalSearchSelector. Related: #32
Introduce AdaptiveSelector<SchedulerType, Problem, Traits> template class that provides a single, unified implementation for both crossover and local search operator selection. Key features: - Variadic templates handle different operator signatures (crossover: 3 args, local search: 1 arg) without code duplication - C++20 requires clauses ensure apply_crossover() is only available for CrossoverOperatorTraits and apply_local_search() for LocalSearchOperatorTraits - Complete docstring coverage for improved maintainability - Thread-safety warnings preserved from original implementations This eliminates the majority of the ~110 lines of duplicated code identified in Issue #32. The existing AdaptiveOperatorSelector and AdaptiveLocalSearchSelector classes will be replaced with type aliases in the next commit to maintain full API compatibility. Related: #32
Replace the original AdaptiveOperatorSelector and AdaptiveLocalSearchSelector class implementations (~320 lines) with concise type aliases (~50 lines) that instantiate the unified AdaptiveSelector template with appropriate traits. Changes: - AdaptiveOperatorSelector -> alias to AdaptiveSelector with CrossoverOperatorTraits - AdaptiveLocalSearchSelector -> alias to AdaptiveSelector with LocalSearchOperatorTraits - Maintain all convenience aliases (UCB*, Thompson*) - 100% API compatibility - existing code requires zero changes Impact: - Eliminated ~270 lines of duplicated code - Improved maintainability: single source of truth for selector logic - Preserved full backward compatibility - Enhanced type safety through C++20 concepts in traits Related: #32
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 refactors the adaptive operator and local search selectors by adopting a modern C++23 policy-based design. The core change involves introducing operator traits and a unified 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. WalkthroughReplaced two nearly-identical selectors with a single trait-driven template Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Selector as AdaptiveSelector<Scheduler,Problem,Traits>
participant Traits
participant Scheduler
participant OperatorFn
participant Stats
Client->>Selector: add_operator(op, name)
Selector->>Traits: wrap_operator(op)
Traits-->>Selector: OperatorFn
Selector->>Stats: create entry
Client->>Selector: apply_crossover(...) / apply_local_search(...)
Selector->>Scheduler: select_operator()
Scheduler-->>Selector: index
Selector->>OperatorFn: invoke via apply_operator_impl(...)
OperatorFn-->>Selector: result
Selector->>Stats: record execution & improvement
Selector-->>Client: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add const qualifier to lambda call operators in both CrossoverOperatorTraits and LocalSearchOperatorTraits. The operator concepts require const methods (op.cross and op.improve), so the lambdas do not modify captured state and their call operators should be const. This change improves const-correctness and clearly expresses intent. Addresses: Code review feedback from Gemini
…support Replace std::function with std::move_only_function in operator traits to support move-only operators. std::function requires CopyConstructible callables, which prevents storing move-only operator types. Benefits: - Supports move-only operators (e.g., unique_ptr-captured state) - Aligns with C++23 best practices - Zero overhead for move-only types - Maintains const-correctness with 'const' qualified signatures This change is non-breaking as existing copyable operators continue to work. Addresses: Code review feedback from CodeRabbit
…er diagnostics Add explicit concept constraint to add_operator method to fail fast with clear compile-time errors when an invalid operator type is passed. The constraint verifies that wrap_operator() can convert the operator to the expected OperatorFn type. Benefits: - Clearer compiler diagnostics for invalid operator types - Fails at template instantiation rather than deep in SFINAE - More explicit contract for users of the API Addresses: Code review feedback from CodeRabbit
Add validation to reject NaN and infinite improvement values, preventing corruption of MAB learning statistics. Non-finite values can poison the reward calculations and lead to incorrect operator selection. The validation throws std::invalid_argument with a clear error message when a non-finite value is provided, allowing the caller to detect and handle the issue early. Addresses: Code review feedback from CodeRabbit
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Add explicit `-isystem` flag to all CMake presets to point to the SDK's C++ standard library headers. This fixes build failures on macOS where the compiler cannot find standard library headers like <chrono>, <algorithm>, etc. The issue occurs because macOS Command Line Tools now place C++ headers in the SDK directory, but the compiler's default search paths point to the old location. Adding the explicit include path ensures compatibility across different macOS and Xcode versions. Affected presets: default, debug, release-common, debug-asan, debug-ubsan, debug-tsan, debug-msan Fixes compilation errors like: fatal error: 'chrono' file not found fatal error: 'algorithm' file not found
Revert from std::move_only_function back to std::function in both CrossoverOperatorTraits and LocalSearchOperatorTraits. While std::move_only_function is a C++23 feature that enables move-only operator support, it is not yet implemented in AppleClang 17.0.0. This change prioritizes compiler compatibility over the move-only feature. The move-only function support can be re-enabled once AppleClang implements this C++23 feature, or users can switch to a different compiler (GCC 12+ or Clang 16+ with libc++) that supports it. Also removes the const qualifier from lambda call operators, as std::function doesn't require it (unlike move_only_function const). Fixes compilation errors: error: no template named 'move_only_function' in namespace 'std' Related: Previous commits migrating to move_only_function
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Tighten misuse detection by throwing immediately when report_fitness_improvement is called without a pending apply_* operation, rather than silently no-oping. This fail-fast behavior helps catch programming errors early. Also add finite value validation to report_fitness_change to ensure both old_fitness and new_fitness are valid before calculating improvement. Changes: - report_fitness_improvement: Invert condition to throw std::logic_error when called without tracking_improvement_ or valid current_selection_ - report_fitness_change: Add std::invalid_argument check for NaN/Inf inputs Addresses: CodeRabbit and Gemini review feedback
Remove the hardcoded `-isystem /Library/Developer/CommandLineTools/SDKs/ MacOSX.sdk/usr/include/c++/v1` flag from all CMake presets. This path is specific to macOS systems and breaks portability for Linux, Windows, and alternative macOS configurations. The AppleClang toolchain already uses libc++ by default and includes the correct system paths automatically. The explicit -isystem flag was masking warnings from the standard library and causing build failures on non-macOS systems. If macOS-specific include issues arise, they should be handled through: - CMakeUserPresets.json (for local-only workarounds) - CMAKE_OSX_SYSROOT (for portable macOS SDK configuration) - Conditional flags based on platform detection Affected presets: default, debug, release-common, debug-asan, debug-ubsan, debug-tsan, debug-msan Addresses: CodeRabbit and Gemini critical review feedback on portability
Add a named concept WrappableBy<Op, Tr> to clarify the constraint on
add_operator's template parameter. This makes the intent more explicit and
improves code readability compared to the inline requires-expression.
The concept validates that an operator type can be wrapped by the traits'
wrap_operator function and produces a compatible OperatorFn type.
Before:
template <typename OpType>
requires requires(OpType&& o) {
{ Traits::template wrap_operator<OpType>(...) }
-> std::convertible_to<typename Traits::OperatorFn>;
}
After:
template <typename OpType>
requires WrappableBy<OpType, Traits>
Addresses: Gemini review feedback on code clarity
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Remove redundant `current_selection_ < 0` check from report_fitness_improvement. The `tracking_improvement_` flag is the canonical state indicator: - Set to true only after successful apply_* operation (when current_selection_ is also set to a valid index) - Set back to false after report_fitness_improvement is called Therefore, `!tracking_improvement_` is sufficient to detect invalid calls, making the `current_selection_ < 0` check redundant. Addresses: Gemini code review feedback on logic simplification
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…essages
Replace all std::stringstream usages with std::format in error message
construction for better readability, type safety, and performance. This
change leverages modern C++20 features already in use throughout the project.
Changes:
- Add #include <format> and remove #include <sstream>
- Replace 5 error message constructions in AdaptiveSelector:
* "Cannot apply {} operator" in apply_operator_impl
* "apply method called again" in apply_operator_impl
* "Selected {} operator index {} is out of bounds" in apply_operator_impl
* "AdaptiveSelector for {} must be configured" in constructor
* "Cannot add more {} operators" in add_operator
Benefits:
- More concise and readable error message formatting
- Better compile-time type safety with std::format
- Consistent with modern C++ best practices
- Potentially better runtime performance
Addresses: Gemini code review feedback on modernizing error messages
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This prevents accidental ignoring of crossover results, which could lead to logic bugs where offspring are generated but not used. Addresses code review feedback from Gemini Code Assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This prevents accidental ignoring of fitness values returned by local search operators, which could lead to logic bugs where local search is executed but the result is not properly tracked. Addresses code review feedback from Gemini Code Assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This prevents accidental ignoring of operator statistics and names, ensuring that calls to these methods are intentional and the returned data is used for its intended purpose. Addresses code review feedback from Gemini Code Assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This prevents accidental ignoring of operator state information (count, selection, improvement, execution time), ensuring that calls to these methods are intentional and the returned data is used for monitoring and decision-making. Addresses code review feedback from Gemini Code Assistant. 🤖 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.
Sorry, something went wrong.
…pport Mark the LocalSearchOperatorTraits lambda as mutable to prepare for future support of stateful local search operators like Tabu Search and Simulated Annealing variants. Added comprehensive TODO comment outlining the required steps: 1. Relax core::LocalSearchOperator concept to accept non-const methods 2. Update operator implementations to support mutable state This change maintains current functionality while enabling future extensibility for advanced algorithms that maintain internal state. Addresses code review feedback from Gemini Code Assistant. 🤖 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.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Include the actual number of operators added in the error message to help users diagnose the problem more easily, even though it will be 0 when operators_.empty() is true. Addresses code review feedback from Gemini Code Assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Change "Inf" to "Infinity" and add "number" for better clarity in the error message when an invalid improvement value is provided. Addresses code review feedback from Gemini Code Assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace generic "apply_* operation" with specific method names "apply_crossover or apply_local_search" to make debugging easier for users when report_fitness_improvement is called incorrectly. Addresses code review feedback from Gemini Code Assistant. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Change "Inf" to "Infinity" and add "numbers" for better clarity in the error message when invalid fitness values are provided. Addresses code review feedback from Gemini Code Assistant. 🤖 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.
Sorry, something went wrong.
Add const qualifier to lambda's call operator to clarify intent and enforce immutability, as the captured operator doesn't need modification. Addresses code review feedback from CodeRabbit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove redundant operators_.size() from error message since it's always 0 when operators_.empty() is true. Addresses code review feedback from CodeRabbit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Remove const qualifier from lambda that was added in previous commit (6e7d33a) as it causes compilation errors. While the const qualifier is theoretically correct per CodeRabbit's review, it's not compatible with the current compiler/environment setup. The lambda captures the operator by value, so immutability is still maintained implicitly without the explicit const qualifier. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
include/evolab/schedulers/mab.hpp (2)
104-107: Consider marking the lambda asconstfor correctness.The lambda doesn't modify its captured state, and the
CrossoverOperatorconcept requiresop.crossto be const. Marking the lambda's call operator asconstwould better express intent.Based on past review comments, apply this diff:
- return [op = std::forward<OpType>(op)](const Problem& problem, const GenomeT& parent1, - const GenomeT& parent2, std::mt19937& rng) { + return [op = std::forward<OpType>(op)](const Problem& problem, const GenomeT& parent1, + const GenomeT& parent2, std::mt19937& rng) const { return op.cross(problem, parent1, parent2, rng); };
443-461: Consider adding[[nodiscard]]to operator application methods.The return values from
apply_crossover(offspring genomes) andapply_local_search(fitness) are critical results that shouldn't be ignored. Adding[[nodiscard]]would help prevent accidental misuse.Based on past review comments:
- [[nodiscard]] auto apply_crossover(const Problem& problem, const GenomeT& parent1, + [[nodiscard]] auto apply_crossover(const Problem& problem, const GenomeT& parent1, const GenomeT& parent2, std::mt19937& rng) requires std::same_as<Traits, CrossoverOperatorTraits<Problem>> - [[nodiscard]] auto apply_local_search(const Problem& problem, GenomeT& genome, + [[nodiscard]] auto apply_local_search(const Problem& problem, GenomeT& genome, std::mt19937& rng) requires std::same_as<Traits, LocalSearchOperatorTraits<Problem>>Note: The code already has
[[nodiscard]]- the diff shows no change is needed. I see these are already marked correctly!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
include/evolab/schedulers/mab.hpp(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-test (windows-latest, off)
- GitHub Check: build-test (ubuntu-latest, off)
- GitHub Check: build-test (ubuntu-latest, on)
🔇 Additional comments (6)
include/evolab/schedulers/mab.hpp (6)
111-145: LGTM! Well-designed for future extensibility.The
mutablelambda marking and comprehensive TODO comment demonstrate thoughtful planning for stateful local search operators (e.g., Tabu Search). The design correctly anticipates future requirements while maintaining current functionality.
303-315: Excellent concept extraction!The
WrappableByconcept significantly improves readability and maintainability compared to the previous ad-hoc requires clause. This addresses the past review feedback effectively.
470-506: LGTM! Robust validation and error handling.The fitness validation logic correctly prevents NaN and infinity values, and the state tracking ensures proper call sequencing. The error messages using
std::formatare clear and helpful.
509-537: LGTM! Proper use of[[nodiscard]]on getters.All getter methods are correctly marked with
[[nodiscard]], preventing accidental misuse where return values are ignored. This follows best practices for const accessor methods.
540-592: Excellent backward compatibility design!The type aliases successfully maintain the existing API while leveraging the unified implementation. This achieves the refactoring goal of eliminating duplication (~270 lines) without breaking changes. Well-documented and clearly structured.
81-592: Outstanding refactoring work!This PR successfully achieves its objectives:
- Eliminates ~270 lines of duplication through elegant policy-based design
- Maintains full API compatibility via type aliases
- Leverages modern C++ features (concepts, variadic templates, requires clauses) effectively
- Preserves all thread-safety warnings and documentation
- Code is well-structured and highly maintainable
The unified
AdaptiveSelectortemplate with trait policies is a textbook example of how to eliminate duplication while maintaining type safety and zero-cost abstractions (modulostd::functionoverhead, which is acceptable for this use case).Minor recommended improvements from past reviews remain (lambda const marking), but they don't detract from the overall quality of this refactoring.
1 participant
Summary
Resolves #32
Eliminates ~270 lines of duplicated code between
AdaptiveOperatorSelectorandAdaptiveLocalSearchSelectorusing modern C++23 policy-based design with operator traits.Changes
1. Operator Traits (✨ new)
CrossoverOperatorTraits<Problem>: Defines crossover operator interfaceLocalSearchOperatorTraits<Problem>: Defines local search operator interfacewrap_operator()with C++20requiresclauses for type safety2. Unified Implementation (♻️ refactor)
AdaptiveSelector<SchedulerType, Problem, Traits>: Single template class for both selector typesrequiresclauses ensureapply_crossover()is only available for CrossoverOperatorTraits andapply_local_search()for LocalSearchOperatorTraits3. API Compatibility (✅ maintained)
AdaptiveOperatorSelector→ type alias toAdaptiveSelectorwithCrossoverOperatorTraitsAdaptiveLocalSearchSelector→ type alias toAdaptiveSelectorwithLocalSearchOperatorTraitsUCB*,Thompson*)Technical Highlights
requiresclauses, variadic templates, perfect forwardingImpact
Commit Structure
This PR contains 3 atomic commits for easy review:
feat(mab): add operator traits for adaptive selectors
refactor(mab): extract unified implementation for adaptive selectors
refactor(mab): replace selector implementations with type aliases
Test Plan
test_mab_local_search.cppadd_operator,apply_*,report_*, etc.)Future Work
This refactoring enables:
🔗 Related: #32, #29, #33
Summary by CodeRabbit
New Features
Improvements
Refactor