Feat/pipeline refinements#27
Conversation
- Removed hard rejection of identical configurations at the diversity_config stage, allowing candidates to proceed to backtest. - Introduced a new diversity_config_pool method to manage historical and batch configurations for diversity checks. - Updated the _check_diversity_gate method to utilize the new pool and adjusted logic for applying jitter. - Enhanced the candidate evaluation process to include post-quality checks for returns diversity, rejecting only exact duplicates based on a hard threshold. - Added new configuration options for select gate comparisons, allowing for flexibility in metric evaluation (deflated vs. raw). - Updated tests to reflect changes in diversity gate behavior and ensure proper handling of identical candidates. - Improved error messages and logging for lookahead bias checks, providing clearer feedback on failures.
…n, and improve environment variable handling
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the strategy layout to a subdirectory structure (strategies/<name>/strategy.py and config.yaml) while maintaining fallback support for the legacy flat layout. It also introduces several new configuration parameters for the optimization loop and safety gates, including a default DSR-based comparison mode (select_compare_metric), near-tie tolerance, and improved diagnostic messages for lookahead and regime stress test failures. The review feedback highlights three key areas for improvement: correctly resolving the strategy name from its parent directory when using the new layout, falling back to raw metric comparison when the target metric is not Sharpe in deflated mode, and adding numerical stability guards (capping and thresholding) to the tradeoff scale factor calculation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -47,7 +47,11 @@ def evaluate( | |||
| raise typer.Exit(code=1) | |||
|
|
|||
| strategy_name = strategy_path.stem | |||
There was a problem hiding this comment.
When using the new subdirectory layout, the strategy file is named strategy.py. Using strategy_path.stem directly results in the strategy name being resolved as "strategy" for all such strategies, which clutters reports and leaderboards. It is better to fall back to the parent directory's name when the stem is "strategy".
| strategy_name = strategy_path.stem | |
| strategy_name = strategy_path.parent.name if strategy_path.stem == "strategy" else strategy_path.stem |
| if compare_metric == "deflated": | ||
| val = report.deflated_sharpe | ||
| else: | ||
| return _get_in_sample_metric_val(report, target_metric) |
There was a problem hiding this comment.
When compare_metric is set to "deflated" but the target_metric is not TargetMetric.SHARPE (e.g., SORTINO or IR), comparing the candidate's deflated_sharpe against the baseline's deflated_sharpe is mathematically incorrect and violates the user's optimization target. We should automatically fall back to raw in-sample metric comparison when the target metric is not Sharpe.
if compare_metric == "deflated" and target_metric == TargetMetric.SHARPE:\n val = report.deflated_sharpe\n else:\n return _get_in_sample_metric_val(report, target_metric)| if compare_metric == "deflated": | ||
| base_raw = baseline.observed_sharpe | ||
| base_dsr = baseline.deflated_sharpe | ||
| scale = (base_dsr / base_raw) if base_raw > 0.0 else 1.0 | ||
| tradeoff_term *= scale |
There was a problem hiding this comment.
If base_raw is extremely close to zero, the division base_dsr / base_raw can blow up, leading to an excessively large scale factor and an unstable tradeoff threshold. Additionally, if base_dsr is negative, the scale factor becomes negative, which reverses the tradeoff logic. Capping the scale factor between 0.0 and 1.0 and adding a safe threshold for base_raw prevents these numerical stability issues.
if compare_metric == "deflated":\n base_raw = baseline.observed_sharpe\n base_dsr = baseline.deflated_sharpe\n scale = max(0.0, min(1.0, base_dsr / base_raw)) if base_raw > 1e-6 else 0.0\n tradeoff_term *= scale
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the strategy layout from a flat structure to a subdirectory-based layout (strategies/<name>/strategy.py and config.yaml), updating the CLI commands, documentation, and tests to support this new structure while maintaining backward compatibility. It also introduces new configuration parameters for the safety gates and optimization loop, including near-tie tolerances and options to compare candidates using raw or deflated metrics. The review feedback identifies a high-severity issue in the evaluate command where relative strategy paths can resolve to empty or . parent names, breaking fallback configuration lookups, and provides a code suggestion to resolve the path first.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| strategy_name = strategy_path.parent.name if strategy_path.stem == "strategy" else strategy_path.stem | ||
| config_path = strategy_path.parent / "config.yaml" | ||
| if not config_path.exists(): | ||
| config_path = strategy_path.parent / f"{strategy_name}.yaml" | ||
| if not config_path.exists(): | ||
| config_path = settings.configs_dir / f"{strategy_name}.yaml" | ||
| if not config_path.exists(): | ||
| config_path = strategy_path.resolve().parent.parent / "configs" / f"{strategy_name}.yaml" |
There was a problem hiding this comment.
If strategy_path is passed as a relative path (for example, just strategy.py when running the command from within the strategy's subdirectory), strategy_path.parent.name will resolve to an empty string "" or ".". This causes strategy_name to be empty, which breaks all subsequent fallback config file lookups.
Resolving the path first via .resolve() ensures that the parent directory name is correctly extracted even when a relative path is provided.
| strategy_name = strategy_path.parent.name if strategy_path.stem == "strategy" else strategy_path.stem | |
| config_path = strategy_path.parent / "config.yaml" | |
| if not config_path.exists(): | |
| config_path = strategy_path.parent / f"{strategy_name}.yaml" | |
| if not config_path.exists(): | |
| config_path = settings.configs_dir / f"{strategy_name}.yaml" | |
| if not config_path.exists(): | |
| config_path = strategy_path.resolve().parent.parent / "configs" / f"{strategy_name}.yaml" | |
| resolved_path = strategy_path.resolve() | |
| strategy_name = resolved_path.parent.name if resolved_path.stem == "strategy" else resolved_path.stem | |
| config_path = resolved_path.parent / "config.yaml" | |
| if not config_path.exists(): | |
| config_path = resolved_path.parent / f"{strategy_name}.yaml" | |
| if not config_path.exists(): | |
| config_path = settings.configs_dir / f"{strategy_name}.yaml" | |
| if not config_path.exists(): | |
| config_path = resolved_path.parent.parent / "configs" / f"{strategy_name}.yaml" |
1 participant
No description provided.