Fix the stats! - #57
Conversation
Added typed forecasting contracts and fit statuses in [contracts.py](/home/sean/Documents/Github/data_forecasting_agent/data_forecaster/backend/forecasting/contracts.py). Added explicit ok, degraded, failed, and not_estimable states. Replaced fabricated zero-error fallbacks with unavailable metrics. Centralized RMSE, MAE, MAPE, WAPE, and MASE calculations. MAPE is now unavailable when actual values contain zero. Added consistent WAPE and MASE calculations to model holdouts. Degraded models can no longer enter model ranking. The pipeline fails explicitly when no model has valid evaluation evidence. Added forecast status information to the API schema.
All four model adapters (ARIMA, SARIMA, Holt-Winters, EWMA) now return ForecastAdapterResult instead of loose dictionaries. The forecasting agent uses typed attribute access throughout and transitional dict lookups are removed. Key changes: - ARIMA/SARIMA preserve with_intercept configuration through refit - Holt-Winters selects additive/multiplicative seasonal on the training split only (fixes test-data leakage in model-form selection) - EWMA estimates alpha by minimizing one-step SSE on the training split instead of using a fixed alpha=0.3; uses centralized metrics instead of the mislabeled perform_rolling_origin_validation - fitted_configuration populated with order, seasonal_order, trend, intercept, seasonal type/period, alpha, and initialization provenance - _calculate_additional_metrics removed (dead code); WAPE/MASE now computed centrally in forecasting/metrics.py - _has_required_metrics operates on ForecastAdapterResult typed objects - Deterministic fallback selection uses lowest RMSE (LLM never ranks) - Updated test_forecasting_metrics.py for typed results Validation: 105 data_forecaster tests passed, 8 metrics tests passed, compileall passed, git diff --check passed.
Deterministic synthetic fixtures (forecasting/fixtures.py) covering: - constant and near-constant series - stationary AR(1) and random walk - additive and multiplicative seasonality - trend without seasonality - zeros and negative values - missing and duplicate timestamps - short seasonal series (< 2 cycles) - isolated anomalies and structural breaks All fixtures use a fixed seed (42) for reproducibility. Failure-state tests (tests/test_forecast_failure_states.py) verify: - Failed/degraded/not_estimable models cannot win ranking - Missing holdout metrics remain None (never zero) - Short-series persistence output is explicitly not_estimable - No fabricated evaluation after fitting exception - All ForecastAdapterResult objects serialize to JSON - Fitted configuration (order, seasonal_order, trend, alpha) survives refit Regression fixture tests (tests/test_forecast_fixtures.py) verify: - Every fixture is deterministic across calls - Each fixture has expected statistical properties - All four adapters survive every fixture without crashing Tests will be validated in the final batch run.
The function perform_rolling_origin_validation performed only a single terminal holdout split, not rolling-origin validation. Renamed to terminal_holdout_validation with accurate docstring. No backward-compatible alias retained (greenfield project). Phase 2 will replace this with a proper expanding-window backtesting service. No code currently imports this function — all four adapters now use centralized metrics directly.
Document all completed R1 tasks: typed contracts, centralized metrics, typed adapter migration, forecasting agent cleanup, obsolete metric logic removal, regression fixtures, and failure-state tests. List remaining R1 work (nullable consumer hardening, test stall diagnosis, final validation).
All report builders, renderers, visualizations, and agents now handle nullable metrics (rmse/mae/mape/wape/mase) safely: - report/models.py: ForecastMetrics and ModelComparisonEntry rmse/mae/mape are now float | None; added format_metric() helper that returns 'not available' for None/NaN/inf values - report/builder.py: _compute_confidence, _compute_health_indicators, _build_forecast_metrics, and _build_model_comparison all guard None metrics before comparison/formatting; model comparison entries no longer mask unavailable metrics as 0.0 - report/dashboard.py: primary_risk guards None mape before comparison - report/renderers/html_renderer.py: model comparison table uses format_metric() instead of bare :.4f formatting - report/renderers/markdown_renderer.py: removed _finite_or_zero (which masked None as 0.0); uses format_metric() for all metric columns - utils/visualization.py: chart title handles None mape/rmse gracefully - agents/report_generation_agent.py: visual strategy MAPE check guards None - agents/model_selection_agent.py: _format_metrics_text handles None/NaN metrics as 'not available' instead of formatting nan Also updated implementation_phases.md with R4 skip note (Phases 6-7).
…tches - arima_model.py/sarima_model.py: Replace getattr(full_model.model, 'trend', None) with trend='c' if with_intercept else 'n' — pmdarima ARIMA objects don't expose a .model attribute - fixtures.py: Fix series names for missing_timestamps and duplicate_timestamps to match their ALL_FIXTURES keys
instead of hard coded
Mariadb support
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR replaces loosely structured forecasting and diagnostics with typed evidence contracts, rolling-origin evaluation, deterministic model selection, explicit failure states, interval provenance, validated narratives, updated report rendering, and expanded regression coverage across backend and frontend workflows. ChangesForecasting pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
tests/test_model_retry_consistency.py (1)
1-223: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winFix
black --checkformatting failure blocking CI.Both CI jobs report
black --checkwould reformat this file. Line 188 (def fake_forecast(...) -> tuple[ForecastResult, dict[str, dict[str, float]]]:) alone is well past the 88-character limit. Test logic itself is otherwise correct and consistent with the mocked pipeline behavior. Runblack tests/test_model_retry_consistency.pyto fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_model_retry_consistency.py` around lines 1 - 223, Format tests/test_model_retry_consistency.py with Black, especially the long fake_forecast definition inside test_retry_preserves_exclusion_and_synchronizes_final_model, so it passes black --check without changing the test behavior.Sources: Coding guidelines, Pipeline failures
tests/test_forecast_fixtures.py (1)
155-186: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCI failure:
test_holt_winters_survives_fixture[zeros]breaks the length invariant.The test unconditionally asserts
len(result.forecast) == FORECAST_HORIZON, but Holt-Winters legitimately returnsNOT_ESTIMABLEwith an empty forecast for the all-zero fixture. The docstring says this is a "survival" test (no unhandled exception), not a full-length-forecast guarantee — the assertion is stronger than the stated intent and needs to account for non-OK statuses.🐛 Suggested fix
`@pytest.mark.parametrize`("name", sorted(ALL_FIXTURES)) def test_holt_winters_survives_fixture(self, name: str) -> None: fn = ALL_FIXTURES[name] series = fn() result = fit_holt_winters(series, FORECAST_HORIZON) assert isinstance(result, ForecastAdapterResult) - assert len(result.forecast) == FORECAST_HORIZON + if result.status in (ForecastFitStatus.OK, ForecastFitStatus.DEGRADED): + assert len(result.forecast) == FORECAST_HORIZON + else: + assert len(result.forecast) == 0The same unconditional-length assumption exists in
test_arima_survives_fixture,test_ewma_survives_fixture, andtest_sarima_survives_fixture— worth applying the same status-aware assertion there for consistency, even though they aren't currently failing in CI.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_forecast_fixtures.py` around lines 155 - 186, Update the fixture survival tests test_arima_survives_fixture, test_ewma_survives_fixture, test_holt_winters_survives_fixture, and test_sarima_survives_fixture to assert FORECAST_HORIZON length only when the ForecastAdapterResult status is OK; allow non-OK outcomes such as NOT_ESTIMABLE with an empty forecast while still verifying no exception and preserving the existing result-type checks.Source: Pipeline failures
data_forecaster/backend/forecasting/fixtures.py (1)
1-284: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun
blackto fix CI formatting failure.CI reports this file would be reformatted by
black --check; e.g., line 197 (full = pd.Series(np.arange(n, dtype=float), index=_index(n), name="missing_timestamps")) exceeds the 88-character limit required by the Google Python Style Guide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/fixtures.py` around lines 1 - 284, Run Black on the fixture module and apply its formatting changes, especially the overlong Series construction in missing_timestamps_series and any other lines exceeding Black’s rules. Preserve all fixture behavior and values.Sources: Coding guidelines, Pipeline failures
data_forecaster/backend/report/builder.py (1)
1699-1719: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGuard nullable forecast metrics in
_build_appendix
rmse,mae, andmapeare nullable onForecastResult, so the unguardedround(...)calls here will raiseTypeErrorfor valid incomplete metrics.mape_quality()also assumes a float and needs aNonepath, or the call site should skip it whenmapeis absent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/builder.py` around lines 1699 - 1719, The `_build_appendix` method must handle nullable `forecast.rmse`, `forecast.mae`, and `forecast.mape` without calling `round` on `None`. Guard each metric consistently with the existing `wape` and `mase` handling, and only call `mape_quality` when `forecast.mape` is present; otherwise store `None`.
🟡 Minor comments (9)
data_forecaster/backend/forecasting/contracts.py-163-218 (1)
163-218: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
ResidualDiagnosticsResultdocstring is missing four documented attributes.
weighted_interval_score,interval_coverage_by_horizon,interval_width_by_horizon, andwinkler_score_by_horizon(lines 212-215) aren't listed in the class docstring'sAttributessection. As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."📝 Proposed docstring addition
coverage_estimable: Whether coverage could be estimated from data. + weighted_interval_score: Single-level weighted interval score. + interval_coverage_by_horizon: Empirical coverage keyed by horizon step. + interval_width_by_horizon: Average interval width keyed by horizon step. + winkler_score_by_horizon: Mean Winkler score keyed by horizon step. warnings: Diagnostics-specific warnings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/contracts.py` around lines 163 - 218, Update the ResidualDiagnosticsResult class docstring’s Attributes section to document weighted_interval_score, interval_coverage_by_horizon, interval_width_by_horizon, and winkler_score_by_horizon, matching their field meanings and existing type behavior.Source: Coding guidelines
data_forecaster/backend/.env.example-18-21 (1)
18-21: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove extra blank line flagged by dotenv-linter.
dotenv-linter reports an
ExtraBlankLineat line 18.🧹 Proposed fix
- - # Sentence-transformers model for RAG embeddings (HuggingFace model ID). EMBED_MODEL=sentence-transformers/all-MiniLM-L6-v2 +🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/.env.example` around lines 18 - 21, Remove the extra blank line immediately before the EMBED_MODEL declaration in the environment example, preserving the comment and variable assignment unchanged.Source: Linters/SAST tools
data_forecaster/backend/report/narrative.py-505-510 (1)
505-510: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPeak value not formatted consistently with the rest of the narrative.
peak_valueis interpolated raw (e.g.,441.123456789), whilepct_change/rmse/mapeelsewhere use fixed precision (.1f/.4f). This can produce visually inconsistent, overly precise numbers in the fallback report text.💚 Proposed fix
- f" A temporary seasonal peak of {peak_value} is projected" + f" A temporary seasonal peak of {peak_value:.1f} is projected"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/narrative.py` around lines 505 - 510, Update the peak_text construction to format peak_value with the same fixed numeric precision convention used by the surrounding narrative metrics, while preserving the existing None fallback and peak_date interpolation.data_forecaster/frontend/templates/main/forecast.html-17-25 (1)
17-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat non-finite CI bounds as unavailable
row.lower_ci is none/row.upper_ci is nonemissesnan/inf, so the table can still render non-finite bounds. Normalize these toNoneindata_forecaster/frontend/blueprints/main/routes.pyor extend the Jinja guard before rendering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/frontend/templates/main/forecast.html` around lines 17 - 25, Update the forecast interval availability handling used by the template’s interval_state logic to treat non-finite lower_ci and upper_ci values, including NaN and infinity, as unavailable. Prefer normalizing these bounds to None in the route that builds forecast_rows; otherwise extend the Jinja guard so non-finite values set bounds_available to false before rendering.data_forecaster/backend/utils/preflight.py-299-309 (1)
299-309: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove now-unused
outlier_strategyassignment.
outlier_strategyis computed/reassigned but never referenced afterward — outlier handling was intentionally removed from this function (per the added comment), leaving dead code that SonarCloud correctly flags.🧹 Proposed cleanup
- outlier_strategy = options.get("outlier_strategy", "None") - if outlier_strategy == "Let AI Decide": - # Diagnostics may flag anomalies, but automatic full-series clipping - # would leak future distributional information into backtests. - outlier_strategy = "None" # Model-affecting preprocessing is intentionally deferred. Rolling-origin # evaluation fits imputation, clipping, and smoothing independently within # each training window; the production refit applies them to full history # only after deterministic selection. if missing_strategy == "drop": series = series.dropna()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/utils/preflight.py` around lines 299 - 309, Remove the unused outlier_strategy retrieval and “Let AI Decide” reassignment block from the preflight function, leaving the missing_strategy handling and existing preprocessing comments intact.Source: Linters/SAST tools
data_forecaster/frontend/services/pdf_service.py-32-41 (1)
32-41: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAdd a fallback font for report text.
_sanitizeonly maps a few emoji, so arbitrary Unicode in LLM-generated content can still render as missing glyphs in the PDF. Register a fallback font or broaden sanitization so those characters are preserved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/frontend/services/pdf_service.py` around lines 32 - 41, Update the PDF text handling around _PDF_SYMBOL_FALLBACKS and _sanitize to support arbitrary Unicode glyphs, preferably by registering and using a fallback font for report text; otherwise broaden sanitization beyond the current emoji mappings while preserving readable characters in LLM-generated content.Source: MCP tools
data_forecaster/tests/test_report_renderers.py-285-285 (1)
285-285: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winXSS (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
Reachability: Unreachable
Enable
autoescapeto match Flask's Jinja defaults.Both
Environment(loader=FileSystemLoader(...))calls default toautoescape=False, unlike Flask's Jinja environment (which auto-escapes.htmltemplates). Since these tests rendermain/report.html/main/forecast.htmloutside the Flask app context, they won't catch a real unescaped-output regression in production.🔒 Suggested fix
- environment = Environment(loader=FileSystemLoader(template_root)) + environment = Environment( + loader=FileSystemLoader(template_root), autoescape=True + )(and similarly for the second
Environment(...)at line 329)Also applies to: 329-332
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/tests/test_report_renderers.py` at line 285, Enable Jinja autoescaping in both test environments created in the report-rendering tests, including the environments loading the report and forecast templates. Configure each Environment to autoescape HTML templates, matching Flask’s defaults while preserving the existing FileSystemLoader setup.Source: Linters/SAST tools
data_forecaster/backend/forecasting/selection_policy.py-265-271 (1)
265-271: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
x or float("inf")treats an RMSE of exactly0.0as unavailable.
c.rmse or float("inf")andbest_baseline.rmse or float("inf")coerce a perfectRMSE == 0.0tofloat("inf")because0.0is falsy in Python — this is aNone-check written as a truthiness-check. A model with a perfect fit would be treated as the worst candidate inmin(baselines, key=lambda c: c.rmse or float("inf"))and would displayinfin therankinglist.🐛 Suggested fix
- best_baseline = min(baselines, key=lambda c: c.rmse or float("inf")) + best_baseline = min( + baselines, key=lambda c: c.rmse if c.rmse is not None else float("inf") + ) ... - ranking = [(c.name, c.rmse or float("inf")) for c in ranked] + ranking = [ + (c.name, c.rmse if c.rmse is not None else float("inf")) for c in ranked + ]Also applies to: 326-326
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/selection_policy.py` around lines 265 - 271, Update the baseline RMSE handling in the selection logic, including the ranking construction around best_baseline, to treat only None as unavailable rather than using truthiness; preserve RMSE values of 0.0 when selecting and displaying candidates. Apply the same explicit None check at the additional occurrence near the ranking logic.data_forecaster/backend/report/renderers/html_renderer.py-220-227 (1)
220-227: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAvoid appending
%afterformat_metricfor percentage cells.format_metricreturnsnot availablefor missing/non-finite values, so both percentage columns can render asnot available%.wapeis already scaled inreport/builder.py, so the stale fallback note can be removed or updated.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/renderers/html_renderer.py` around lines 220 - 227, Update the percentage cells in the row rendering block to avoid appending a literal “%” when format_metric returns “not available”; ensure valid wape and mape values retain the intended percentage display. Remove or revise the stale fallback comment, and preserve wape’s existing scaling from report/builder.py.
🧹 Nitpick comments (26)
data_forecaster/backend/agents/model_selection_agent.py (3)
622-638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstrings missing Args/param coverage for new public API surface.
build_model_rejection_reasons(new public helper) has only a one-line docstring with noArgs/Returnssections despite taking four parameters and containing branching logic.run_model_selection_agent's docstringArgssection doesn't document the newly addedloss_preferenceparameter (Line 953).As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."
📝 Proposed fix
def build_model_rejection_reasons( selected_model: str, stat_result: StatisticalResult, all_metrics: dict[str, dict[str, float]] | None = None, excluded_models: list[str] | None = None, ) -> dict[str, str | None]: - """Build final rejection reasons aligned to the production model.""" + """Build final rejection reasons aligned to the production model. + + Args: + selected_model: The model ultimately selected for production use. + stat_result: Output of the statistical analysis agent. + all_metrics: Optional dict of per-model error metrics. + excluded_models: Optional list of models excluded during retry. + + Returns: + A dict mapping each model name to its rejection reason, or ``None`` + for the selected model. + """all_metrics: Optional dict of actual model error metrics from the prior forecasting run, used to make an evidence-based reselection during retry. + loss_preference: Loss metric used by the deterministic ranking policy + when ``all_metrics`` is available (e.g. ``"mase"``).Also applies to: 948-976
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/model_selection_agent.py` around lines 622 - 638, Expand the Google-style docstring for build_model_rejection_reasons to document selected_model, stat_result, all_metrics, and excluded_models under Args and describe its dict return value under Returns. Update run_model_selection_agent’s Args section to document the loss_preference parameter, preserving the existing descriptions and behavior.Source: Coding guidelines
824-865: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant re-import of
ForecastAdapterResult.
ForecastAdapterResultis already imported at module level (Line 31); the local import inside_build_adapter_resultre-imports it alongsideForecastFitStatus/ForecastMetrics. MoveForecastFitStatusandForecastMetricsto the top-level import block and drop the duplicate.♻️ Proposed fix
from forecasting.contracts import ForecastAdapterResult +from forecasting.contracts import ForecastFitStatus, ForecastMetrics) -> "ForecastAdapterResult": """Build a :class:`ForecastAdapterResult` from a metrics dict. @@ Returns: A :class:`ForecastAdapterResult` with typed metrics. """ - from forecasting.contracts import ( - ForecastAdapterResult, - ForecastFitStatus, - ForecastMetrics, - ) - rmse = _finite_or_none(metrics.get("RMSE"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/model_selection_agent.py` around lines 824 - 865, Remove the local import block from _build_adapter_result, retaining the existing module-level ForecastAdapterResult import. Add ForecastFitStatus and ForecastMetrics to that top-level import block, and continue using them unchanged when constructing the result.Source: Coding guidelines
647-657: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent finiteness check:
np.isfinitevsmath.isfinite.
_format_metric_valueusesnp.isfinitewhile_finite_or_none(added in the same PR) uses the already-importedmath.isfinitefor the identical scalar check. Prefermath.isfinitehere too — it's stdlib, already imported, and there's no vectorization benefit for a single scalar.♻️ Proposed fix
- if value is None or not np.isfinite(value): + if value is None or not math.isfinite(value): return _NOT_AVAILABLEAlso applies to: 817-821
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/model_selection_agent.py` around lines 647 - 657, Update _format_metric_value to use the already-imported math.isfinite for its scalar finiteness check, matching _finite_or_none, and apply the same replacement at the additional occurrence around the referenced code. Preserve the existing None handling and formatting behavior.data_forecaster/backend/agents/statistical_analysis_agent.py (1)
29-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoosen
objecttyping on_status_mapsfor a safer contract.
item.status.value/item.warningsare accessed on values typed as bareobject, which a type checker cannot verify has these attributes. A smallProtocol(e.g.status: DiagnosticStatus+warnings: list[str]) would type-check correctly againstSeasonalityEvidence,StationarityEvidence,TrendEvidence,AnomalyEvidence, andChangePointEvidence.♻️ Proposed fix
+from typing import Protocol + + +class _DiagnosticEvidence(Protocol): + status: object + warnings: list[str] + + def _status_maps( - *evidence: tuple[str, object] + *evidence: tuple[str, _DiagnosticEvidence] ) -> tuple[dict[str, str], dict[str, list[str]]]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/statistical_analysis_agent.py` around lines 29 - 38, Define a small structural Protocol for evidence items with status: DiagnosticStatus and warnings: list[str], then change _status_maps to accept evidence values conforming to that Protocol instead of object. Keep the existing status and warning extraction behavior unchanged so all listed evidence types satisfy the shared contract.data_forecaster/backend/forecasting/diagnostics.py (2)
239-239: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo Ruff nits: redundant
int()and list-concat.Line 239:
round(period)on a float already returnsint, so the outerint(...)is redundant (RUF046). Line 803: prefer[*left_cps, split_abs, *right_cps]over list concatenation (RUF005).🧹 Proposed fixes
- int_period = int(round(period)) + int_period = round(period)- return left_cps + [split_abs] + right_cps + return [*left_cps, split_abs, *right_cps]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/diagnostics.py` at line 239, In the diagnostics code, remove the redundant int() wrapper around round(period) when assigning int_period, and update the list construction near the split_abs handling to use iterable unpacking ([*left_cps, split_abs, *right_cps]) instead of list concatenation. Preserve the existing values and ordering.Source: Linters/SAST tools
930-950: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
assess_arch_effectsnever removes seasonal variance before testing for ARCH effects.
_compute_adjusted_residuals(..., 1)is called with a hardcoded period of 1 (pure linear detrend only), whiledetect_anomaliesin this same module deliberately receives the detectedseasonality.selected_periodfor the same residual-computation helper. For seasonal series, testing raw seasonally-unadjusted residuals for conditional heteroskedasticity can misclassify seasonal variance clusters as ARCH effects, since the function isn't given a way to accept a period.♻️ Proposed fix
-def assess_arch_effects(series: pd.Series, lags: int = 5) -> dict[str, object]: +def assess_arch_effects( + series: pd.Series, seasonal_period: int = 1, lags: int = 5 +) -> dict[str, object]: """Test adjusted residuals for conditional heteroskedasticity.""" from statsmodels.stats.diagnostic import het_arch - residuals = _compute_adjusted_residuals(series.dropna().astype(float), 1) + residuals = _compute_adjusted_residuals( + series.dropna().astype(float), seasonal_period + )And thread the caller's
seasonality.selected_periodthrough, e.g. inrun_statistical_agent:assess_arch_effects(values, seasonal_period=seasonality.selected_period).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/diagnostics.py` around lines 930 - 950, Update assess_arch_effects to accept a seasonal_period parameter and pass it to _compute_adjusted_residuals instead of the hardcoded period 1. Thread the detected seasonality.selected_period from its caller, including run_statistical_agent, so ARCH testing uses seasonally adjusted residuals.data_forecaster/backend/forecasting/backtesting.py (2)
313-478: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce
evaluate_candidatecognitive complexity (SonarCloud: 22 vs 15 allowed).The final-test-window block (already flagged above) is a natural candidate for extraction into a private helper (e.g.
_evaluate_final_test_window(name, series, folds, candidate_fn, config, warnings)returning(final_test_metrics, successful_origins_adjustment)), which would also make the clamp fix above easier to apply and test in isolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/backtesting.py` around lines 313 - 478, Reduce cognitive complexity in evaluate_candidate by extracting the final-test-window setup, fold processing, and metric calculation into a private helper such as _evaluate_final_test_window. Have the helper return the final ForecastMetrics and any required successful-origin adjustment, while preserving the existing untouched-window behavior, failure handling, and clamped training boundary. Replace the inline block in evaluate_candidate with the helper call.Source: Linters/SAST tools
51-80: 🚀 Performance & Scalability | 🔵 TrivialBootstrap metric intervals run 500 resamples per candidate on the evaluation path.
_bootstrap_metric_intervalscallscalculate_forecast_metricsup to 500 times per candidate, andevaluate_candidatesruns this once per model (ARIMA/SARIMA/Holt-Winters/EWMA/baselines, etc.). For larger pooled fold sizes or many candidates this is a meaningful, synchronous CPU cost on whatever request path callsevaluate_candidates. Worth confirming this runs off the request thread (background job/async) or thatrepetitions/candidate count is bounded for production series sizes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/backtesting.py` around lines 51 - 80, Bound the synchronous bootstrap cost in _bootstrap_metric_intervals and its evaluate_candidates call path for production workloads. Enforce a suitable limit on repetitions and/or candidate evaluation count, or move this interval computation off the request thread while preserving deterministic intervals and existing metric behavior.data_forecaster/backend/utils/statistical.py (1)
148-166: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the detrending exception instead of silently swallowing it.
The bare
except Exception: passaround thelinregressdetrend hides any failure from view, per Ruff's S110/BLE001 hints. Catch a narrower exception and log it at debug level so silent detrend failures are diagnosable.🛠️ Proposed fix
if len(values) >= 3: try: slope, intercept, _, _, _ = linregress(x, values) values = values - (slope * x + intercept) - except Exception: # pylint: disable=broad-except - pass + except ValueError as exc: + logger.debug("Periodogram detrend skipped: %s", exc)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/utils/statistical.py` around lines 148 - 166, Update the detrending block in the periodogram function around linregress to catch the specific expected exception rather than Exception, and log that failure at debug level using the module’s existing logger before continuing without detrending. Preserve the current fallback behavior when detrending cannot be applied.Source: Linters/SAST tools
data_forecaster/backend/report/narrative.py (1)
264-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated dash-normalization regex into a shared constant.
The same character class
r"[‐‑‒–—−]"is repeated 4 times across contradiction-checking helpers. A future edit to one copy (e.g., adding another dash variant) without updating the rest would cause inconsistent normalization between these validators.♻️ Proposed fix
+_DASH_VARIANTS_PATTERN = re.compile(r"[‐‑‒–—−]") + def _unexpected_model_references(text: str, expected_model: str) -> list[str]: """Reject forecast prose that names a model other than the fitted model.""" - normalized = re.sub(r"[‐‑‒–—−]", "-", text).lower() + normalized = _DASH_VARIANTS_PATTERN.sub("-", text).lower()(repeat for the other 3 occurrences)
Also applies to: 286-286, 306-306, 397-397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/narrative.py` at line 264, Extract the repeated dash character-class regex into one shared module-level constant in narrative.py, then update the normalization expressions in the contradiction-checking helpers at the occurrences around lines 264, 286, 306, and 397 to reference that constant while preserving the existing lowercase normalization behavior.Source: Linters/SAST tools
data_forecaster/backend/forecasting/fixtures.py (1)
268-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Callable[[], pd.Series]instead of the builtincallable.
callableis a builtin function, not a valid type annotation; static type checkers will flagdict[str, callable]. Usecollections.abc.Callable.♻️ Proposed fix
+from collections.abc import Callable + ... -ALL_FIXTURES: dict[str, callable] = { +ALL_FIXTURES: dict[str, Callable[[], pd.Series]] = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/fixtures.py` around lines 268 - 283, Update the ALL_FIXTURES type annotation to use collections.abc.Callable with the signature Callable[[], pd.Series] instead of the builtin callable, adding the required import while leaving the fixture mappings unchanged.Source: Coding guidelines
data_forecaster/backend/schemas.py (1)
174-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
Literal/StrEnumforselection_methodinstead of free-formstr.The valid values are documented only in a comment; a
Literal["deterministic", "llm", "heuristic", "forced"](mirroring theForecastFitStatusStrEnum already used in this file) would let type checkers catch typos across the multiple producer/consumer files (model_selection_agent.py,builder.py).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/schemas.py` around lines 174 - 177, The selection_method field currently accepts arbitrary strings despite having a fixed set of valid values. Update selection_method in the relevant schema class to use a Literal or StrEnum containing exactly deterministic, llm, heuristic, and forced, following the existing ForecastFitStatus pattern and preserving the default value.data_forecaster/backend/forecasting/selection_policy.py (1)
432-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove
import reto the top of the module.
reis imported inline in both_check_invented_metricsand_check_contradictory_selection; per the repo's import-grouping guideline, standard-library imports should live at module level.Also applies to: 471-471
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/selection_policy.py` at line 432, Move the standard-library `re` import from the inline locations in `_check_invented_metrics` and `_check_contradictory_selection` to the module-level import section at the top of the file, removing both function-local imports.Source: Coding guidelines
data_forecaster/backend/forecasting/metrics.py (1)
55-154: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMetric math verified correct; complexity flagged by SonarCloud (23 vs 15 allowed).
Cross-checked MAPE/WAPE/MASE/RMSSE/sMAPE branches against
tests/test_forecasting_metrics.pyandtests/test_forecast_failure_states.py— all consistent. Consider extracting each metric's computation (e.g._mape,_wape,_mase_rmsse,_smape) into small helpers to reduce cognitive complexity, mirroring the same suggestion forfit_sarima.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/metrics.py` around lines 55 - 154, Reduce the cognitive complexity of calculate_forecast_metrics by extracting the MAPE, WAPE, combined MASE/RMSSE, and sMAPE calculations into focused private helpers. Preserve the existing metric values, unavailable_reasons messages, finite-observation filtering, and ForecastMetrics assembly while keeping calculate_forecast_metrics responsible for orchestration.Source: Linters/SAST tools
data_forecaster/backend/forecasting/sarima_model.py (1)
47-231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCognitive complexity flagged by SonarCloud (21 vs 15 allowed).
Consider extracting the diagnostics assembly (warnings/converged/stationary/invertible) and the
fitted_configurationdict construction into small helper functions to bringfit_sarimaunder the complexity threshold.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/sarima_model.py` around lines 47 - 231, Reduce fit_sarima’s cognitive complexity by extracting the convergence, root diagnostics, and fit_warnings assembly into a focused helper, and moving the fitted_configuration dictionary construction into a separate helper. Update fit_sarima to call these helpers while preserving all existing diagnostic values, warning text, and configuration fields.Source: Linters/SAST tools
data_forecaster/backend/report/builder.py (2)
654-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal variable
confidence_labelshadows the importedreport.rules.confidence_labelfunction.
confidence_labelis imported at module scope (line ~51) and used as a function in_compute_confidence(label = confidence_label(score)). Here, within_build_forecast_metrics, a same-named local string variable is assigned, which is legal (function-local scoping) but confusing — any future edit that tries to call the importedconfidence_label(...)inside this method will silently resolve to the local string instead, or raiseUnboundLocalError/TypeErrordepending on placement.♻️ Proposed rename
- confidence_label = ( + interval_confidence_label = ( "95% (experimental)" if interval_label == "experimental" else _CONFIDENCE_LEVEL ) ... - confidence_level=confidence_label, + confidence_level=interval_confidence_label,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/builder.py` around lines 654 - 665, Rename the local string variable confidence_label in _build_forecast_metrics to a distinct name, such as confidence_text, and update its references in the returned metrics. Preserve the imported report.rules.confidence_label function and its use in _compute_confidence unchanged.
733-747: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstrings weren't updated for the new
forecastparameter (and a new method has none at all).
_build_model_comparison(733-747) gainedforecast: ForecastResult | None = None, but its Args section only documentsall_metricsandmodel_selection._build_assumptions(1247-1261) gainedforecast: ForecastResult, but its Args section only documentsstatisticalandvalidation._selection_rationale(810-816) is a new, non-trivial static method with multi-branch logic but has only a one-line docstring — no Args/Returns.As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."
Also applies to: 810-816, 1247-1261
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/builder.py` around lines 733 - 747, Update the Google-style docstrings for _build_model_comparison and _build_assumptions to document their new forecast parameters with types and meanings. Expand the _selection_rationale docstring to include Args and Returns sections describing its inputs, branching rationale, and returned value. Preserve the existing behavior and document only the method contracts.Source: Coding guidelines
data_forecaster/backend/report/dashboard.py (2)
161-238: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
primary_risk/recommended_actionlogic is re-implemented independently inreport/builder.py, and the copies have already drifted.This module is effectively the canonical implementation of "what risk/action should we show," but
report/builder.pyreimplements pieces of it three times:
- The RMSE-ratio extraction (
pooled_rmse = forecast.selection_metrics.get("rmse")fallback toforecast.rmse, thenrecent_holdout_rmse_ratio(...)) is duplicated verbatim as the module-level_recent_holdout_rmse_ratiohelper inreport/builder.py.- The
has_collection_issueboolean expression here (lines 221-227) is duplicated verbatim inExecutiveReportBuilder._build_recommendations.ExecutiveReportBuilder._build_executive_summaryreimplements a primary-risk/recommended-action block inline, but — unlikeprimary_riskhere — it never checkshas_structural_breaks, so the dashboard's "Primary Risk" widget and the executive summary's "Primary Risk" field can disagree for the same forecast when structural breaks are detected.Since
report/builder.pyalready imports fromreport.rules, consider movingprimary_risk/recommended_action(or at least the RMSE-ratio and collection-issue helpers) intoreport/rules.pyso bothdashboard.pyandbuilder.pyshare one implementation instead of three parallel ones.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/dashboard.py` around lines 161 - 238, Consolidate the canonical primary_risk and recommended_action logic in report.rules, including shared recent-holdout RMSE and collection-issue handling. Update dashboard.py and ExecutiveReportBuilder methods such as _build_recommendations and _build_executive_summary to call these shared functions instead of maintaining duplicate logic. Ensure the builder passes has_structural_breaks so executive-summary and dashboard risk results remain consistent.
112-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
direction_statusis unused; remove the dead helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/dashboard.py` around lines 112 - 118, Remove the unused direction_status helper, including its docstring and conditional logic, while leaving the FORECAST_DIRECTIONS constants and other active forecast behavior unchanged.data_forecaster/backend/forecasting/holt_winters.py (2)
144-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
dataclasses.asdictinstead of raw__dict__.
selected.__dict__works for a non-slotted frozen dataclass but relies on an implementation detail rather than the public dataclasses API.♻️ Proposed fix
- fitted_configuration={"model": "Holt-Winters", **selected.__dict__}, + fitted_configuration={"model": "Holt-Winters", **dataclasses.asdict(selected)},(requires
import dataclasses)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/holt_winters.py` around lines 144 - 149, Update the fitted_configuration construction in the Holt-Winters failure result to use the public dataclasses.asdict API on selected instead of selected.__dict__, adding the required dataclasses import while preserving the existing model entry and configuration contents.
73-96: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftCumulative-residual bootstrap is a simplification for non-random-walk forms.
Sampling i.i.d. fitted residuals and cumulative-summing them across the horizon models random-walk-like error growth; for damped-trend/seasonal forms the true multi-step uncertainty doesn't necessarily grow this way. This is already disclosed via
parameter_uncertainty_included: False, and downstreamcalibrate_interval_widthempirically corrects coverage from backtest folds, so the risk is mitigated. ConsiderExponentialSmoothingResults.simulate()-based intervals as a more rigorous alternative if interval calibration issues surface in practice.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/holt_winters.py` around lines 73 - 96, Retain the current cumulative-residual bootstrap in bootstrap_holt_winters_interval; the limitation is already documented by parameter_uncertainty_included and mitigated by calibrate_interval_width. Do not change the implementation unless interval calibration reveals practical coverage issues, in which case evaluate ExponentialSmoothingResults.simulate()-based intervals as a separate enhancement.data_forecaster/backend/agents/forecasting_agent.py (2)
121-153: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
run_forecasting_agentdocstringArgsdon't document the three new parameters.
loss_preference,preprocessing_options, andexclude_modelswere added to the signature but aren't documented inArgs.As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/forecasting_agent.py` around lines 121 - 153, The run_forecasting_agent docstring is missing Args entries for the newly added loss_preference, preprocessing_options, and exclude_models parameters. Add concise Google-style documentation for each parameter, describing its purpose and optional/default behavior, while preserving the existing documentation for all other arguments.Source: Coding guidelines
713-753: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
_run_backtest_evaluationcognitive complexity flagged by SonarCloud (35 vs. 15 allowed).The eight
_xxx_fnfold-predictor closures plus the constant-series/skew-transform special-casing make this function hard to navigate and unit-test in isolation. Consider extracting the fold predictors to module-level functions (or a smallbacktest_candidateshelper module) so each can be tested independently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/forecasting_agent.py` around lines 713 - 753, Reduce the cognitive complexity of _run_backtest_evaluation by extracting its eight _xxx_fn fold-predictor closures and the constant-series/skew-transform special-case logic into module-level helpers or a dedicated backtest_candidates module. Update _run_backtest_evaluation to compose these helpers while preserving existing candidate behavior and make each extracted predictor independently testable.Source: Linters/SAST tools
data_forecaster/backend/forecasting/arima_model.py (1)
21-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring
Argsare stale relative to the actual signature.
_calculate_metrics(holdout, model, mase_period)documentstrain,test,modelinArgs, but the real parameters areholdout,model,mase_period—mase_periodisn't documented at all, andholdout/modelare untyped.As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions" and "typed public APIs."📝 Proposed fix
-def _calculate_metrics(holdout, model, mase_period: int) -> ForecastMetrics: - """Calculate RMSE, MAE, and MAPE for the given model and test data. - - Args: - train: Training data used for MASE scale. - test: Holdout observations. - model: Trained ARIMA model with a ``predict`` method. - - Returns: - Typed metrics. Unavailable evidence is never encoded as zero. - """ +def _calculate_metrics( + holdout: TerminalHoldout, model: object, mase_period: int +) -> ForecastMetrics: + """Calculate holdout metrics for the given fitted model. + + Args: + holdout: Terminal-holdout train/test split. + model: Trained ARIMA model with a ``predict`` method. + mase_period: Seasonal period for the MASE scale. + + Returns: + Typed metrics. Unavailable evidence is never encoded as zero. + """(use the actual holdout type from
forecasting.evaluation)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/arima_model.py` around lines 21 - 40, Update _calculate_metrics to use the evaluation module’s holdout type and add the appropriate model and mase_period annotations. Rewrite its Google-style Args section to document holdout, model, and mase_period using the actual parameter names and purposes, removing stale train/test entries.Source: Coding guidelines
data_forecaster/backend/forecasting/residual_diagnostics.py (2)
257-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
zip()withoutstrict=over per-fold sequences.
fold_actuals/fold_lower/fold_upper(and similarly at line 366) are expected to always be the same length by construction; addingstrict=Trueturns a silent misalignment (which would corrupt interval metrics) into an immediate, debuggable error.Also applies to: 366-366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/residual_diagnostics.py` at line 257, Update the `zip()` calls in the residual diagnostics loops around the per-fold interval metrics, including the corresponding loop near line 366, to use strict length validation. Ensure `fold_actuals`, `fold_lower`, and `fold_upper` mismatches raise immediately instead of silently truncating, while preserving the existing iteration logic.Source: Linters/SAST tools
278-417: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
analyze_backtest_errorscognitive complexity flagged by SonarCloud (30 vs. 15 allowed).The by-horizon loop (lines 362-389) is the main contributor and could be extracted into a small helper (mirroring
_compute_interval_metrics) to bring this under the complexity budget.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/residual_diagnostics.py` around lines 278 - 417, Reduce cognitive complexity in analyze_backtest_errors by extracting the per-horizon interval metric loop into a focused helper, mirroring _compute_interval_metrics. Have the helper process fold_actuals, fold_lower, fold_upper, and nominal_coverage and return the three by-horizon metric dictionaries; replace the inline loop with a call while preserving existing alignment, filtering, and metric behavior.Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6258d14b-8b7e-4be3-a315-3dca64c51d12
📒 Files selected for processing (62)
data_forecaster/backend/.env.exampledata_forecaster/backend/agents/data_validation_agent.pydata_forecaster/backend/agents/forecasting_agent.pydata_forecaster/backend/agents/model_selection_agent.pydata_forecaster/backend/agents/report_generation_agent.pydata_forecaster/backend/agents/statistical_analysis_agent.pydata_forecaster/backend/agents/statistical_review_agent.pydata_forecaster/backend/core/config.pydata_forecaster/backend/forecasting/arima_model.pydata_forecaster/backend/forecasting/backtesting.pydata_forecaster/backend/forecasting/contracts.pydata_forecaster/backend/forecasting/diagnostics.pydata_forecaster/backend/forecasting/evaluation.pydata_forecaster/backend/forecasting/ewma_model.pydata_forecaster/backend/forecasting/fixtures.pydata_forecaster/backend/forecasting/holt_winters.pydata_forecaster/backend/forecasting/metrics.pydata_forecaster/backend/forecasting/preprocessing.pydata_forecaster/backend/forecasting/residual_diagnostics.pydata_forecaster/backend/forecasting/sarima_model.pydata_forecaster/backend/forecasting/selection_policy.pydata_forecaster/backend/prompts/forecasting_prompt.pydata_forecaster/backend/prompts/general_chat_prompt.pydata_forecaster/backend/prompts/model_selection_prompt.pydata_forecaster/backend/prompts/orchestrator_prompt.pydata_forecaster/backend/prompts/report_generation_prompt.pydata_forecaster/backend/prompts/statistical_analysis_prompt.pydata_forecaster/backend/prompts/statistical_review_prompt.pydata_forecaster/backend/rag/knowledge_base.pydata_forecaster/backend/report/builder.pydata_forecaster/backend/report/dashboard.pydata_forecaster/backend/report/models.pydata_forecaster/backend/report/narrative.pydata_forecaster/backend/report/renderers/html_renderer.pydata_forecaster/backend/report/renderers/markdown_renderer.pydata_forecaster/backend/report/rules.pydata_forecaster/backend/schemas.pydata_forecaster/backend/services/baseline_service.pydata_forecaster/backend/services/pipeline_service.pydata_forecaster/backend/utils/data_cleaning.pydata_forecaster/backend/utils/preflight.pydata_forecaster/backend/utils/statistical.pydata_forecaster/backend/utils/validation.pydata_forecaster/backend/utils/visualization.pydata_forecaster/docker/Dockerfile.flaskdata_forecaster/frontend/blueprints/main/routes.pydata_forecaster/frontend/services/pdf_service.pydata_forecaster/frontend/static/js/app.jsdata_forecaster/frontend/templates/main/forecast.htmldata_forecaster/frontend/templates/main/report.htmldata_forecaster/frontend/templates/main/started.htmldata_forecaster/tests/test_report_builder.pydata_forecaster/tests/test_report_renderers.pydata_forecaster/tests/test_report_rules.pyimplementation_phases.mdtests/test_airline_report_consistency.pytests/test_decision_loss.pytests/test_forecast_failure_states.pytests/test_forecast_fixtures.pytests/test_forecasting_metrics.pytests/test_model_retry_consistency.pytests/test_pdf_service.py
| res = results_store[selected] | ||
| if not res.is_rankable: | ||
| rankable = { | ||
| name: candidate | ||
| for name, candidate in results_store.items() | ||
| if candidate.is_rankable and name not in excluded_models | ||
| } | ||
| if not rankable: | ||
| raise RuntimeError( | ||
| "No forecasting model produced valid evaluation metrics." | ||
| ) | ||
| # Deterministic policy: lowest RMSE wins. The LLM never decides | ||
| # model rankings. | ||
| selected = min( | ||
| rankable, key=lambda name: rankable[name].metrics.rmse or float("inf") | ||
| ) | ||
| res = rankable[selected] | ||
| res = res.model_copy(update={"is_fallback": True}) | ||
| logger.warning( | ||
| "Selected model lacked valid evaluation evidence; falling back to %s", | ||
| selected, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fallback res is updated but results_store[selected] isn't — candidate_results will show stale is_fallback.
res = res.model_copy(update={"is_fallback": True}) rebinds the local res, but results_store[selected] (the dict entry) still points at the original, non-fallback object. Later, candidate_results is built from results_store.items() (lines ~593), so the entry for this exact model will show is_fallback=candidate.is_fallback = False, contradicting forecast_result.is_fallback = res.is_fallback = True for the same selected model. This is a self-inconsistent ForecastResult.
🔧 Proposed fix
res = rankable[selected]
res = res.model_copy(update={"is_fallback": True})
+ results_store[selected] = res
logger.warning(
"Selected model lacked valid evaluation evidence; falling back to %s",
selected,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res = results_store[selected] | |
| if not res.is_rankable: | |
| rankable = { | |
| name: candidate | |
| for name, candidate in results_store.items() | |
| if candidate.is_rankable and name not in excluded_models | |
| } | |
| if not rankable: | |
| raise RuntimeError( | |
| "No forecasting model produced valid evaluation metrics." | |
| ) | |
| # Deterministic policy: lowest RMSE wins. The LLM never decides | |
| # model rankings. | |
| selected = min( | |
| rankable, key=lambda name: rankable[name].metrics.rmse or float("inf") | |
| ) | |
| res = rankable[selected] | |
| res = res.model_copy(update={"is_fallback": True}) | |
| logger.warning( | |
| "Selected model lacked valid evaluation evidence; falling back to %s", | |
| selected, | |
| ) | |
| res = results_store[selected] | |
| if not res.is_rankable: | |
| rankable = { | |
| name: candidate | |
| for name, candidate in results_store.items() | |
| if candidate.is_rankable and name not in excluded_models | |
| } | |
| if not rankable: | |
| raise RuntimeError( | |
| "No forecasting model produced valid evaluation metrics." | |
| ) | |
| # Deterministic policy: lowest RMSE wins. The LLM never decides | |
| # model rankings. | |
| selected = min( | |
| rankable, key=lambda name: rankable[name].metrics.rmse or float("inf") | |
| ) | |
| res = rankable[selected] | |
| res = res.model_copy(update={"is_fallback": True}) | |
| results_store[selected] = res | |
| logger.warning( | |
| "Selected model lacked valid evaluation evidence; falling back to %s", | |
| selected, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/agents/forecasting_agent.py` around lines 405 - 427,
Update the fallback branch in the forecasting selection flow so the copied
result with is_fallback=True replaces the corresponding results_store[selected]
entry, not only the local res variable. Keep res and the selected model’s
results_store record aligned so later candidate_results construction reports the
same fallback state as forecast_result.
There was a problem hiding this comment.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
data_forecaster/backend/agents/model_selection_agent.py (1)
953-976: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument the new
loss_preferenceparameter.The Args block doesn't mention the newly added
loss_preferenceparameter. As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."📝 Proposed docstring fix
all_metrics: Optional dict of actual model error metrics from the prior forecasting run, used to make an evidence-based reselection during retry. + loss_preference: Loss metric used by the deterministic ranking policy + when `all_metrics` is available (e.g. "mase", "rmse", + "wape"). Defaults to "mase".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/model_selection_agent.py` around lines 953 - 976, Update the Google-style Args section of the model-selection function docstring to document the loss_preference parameter, including its purpose and default behavior. Keep the existing documentation for the other parameters unchanged.Source: Coding guidelines
data_forecaster/backend/forecasting/sarima_model.py (1)
73-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
is_fallbackshould only mark short-series downgrades, not all non-seasonal fits.
Whenseasonal_periodis already<= 1,use_seasonalis false even though no fallback happened, so the comparison summary/reporting labels a valid ARIMA fit as a fallback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/sarima_model.py` around lines 73 - 82, Track whether the short-series branch in the seasonal-period setup actually changed the model configuration, and use that state for is_fallback instead of deriving it from use_seasonal. Keep naturally non-seasonal fits with an original seasonal_period <= 1 marked as non-fallback, while marking only downgrades triggered by the len(series) check.data_forecaster/backend/forecasting/arima_model.py (1)
42-208: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCognitive complexity CI check fails on
fit_arima.SonarCloud reports cognitive complexity 21 vs. the allowed 15 (CI failure). Consider extracting the diagnostics block (128-154) and the fitted_configuration construction (190-204) into helper functions to bring this under the threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/arima_model.py` around lines 42 - 208, Reduce cognitive complexity in fit_arima below the allowed threshold by extracting the AR/MA diagnostics logic into a dedicated helper and moving fitted_configuration assembly into another helper. Update fit_arima to call these helpers while preserving existing convergence, stationarity, invertibility, warning, and configuration values.Source: Linters/SAST tools
data_forecaster/backend/agents/statistical_review_agent.py (1)
258-306: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_check_suboptimal_rmseand_check_deterministic_policy_violationduplicate the same computation and will both fire together.Both functions compute an identical
selected_rmse/comparable/best_model/best_rmse/ratio > 1.5check and both emit aseverity="critical", agent="model_selection"flag._check_deterministic_policy_violationonly gates onselection_method == "deterministic", but_check_suboptimal_rmseruns unconditionally for every selection method — so for deterministic selections (the pipeline's primary path per this PR), both fire simultaneously whenever RMSE ratio > 1.5, producing two near-duplicate critical flags inpre_check_flags. This inflates_format_pre_check_flagstext sent to the LLM, duplicates entries in_compute_override_eligibility'soverride_reasons, and shows redundant critical flags to end users in the review report.Make the two checks mutually exclusive based on selection method.
🐛 Proposed fix
_check_explanation_mismatch(model_selection, selected), - _check_suboptimal_rmse(selected, all_metrics), - _check_deterministic_policy_violation(model_selection, all_metrics), + ( + _check_deterministic_policy_violation(model_selection, all_metrics) + if model_selection.selection_method == "deterministic" + else _check_suboptimal_rmse(selected, all_metrics) + ), _check_residual_autocorrelation(forecast_result),Also applies to: 390-476, 505-506
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/statistical_review_agent.py` around lines 258 - 306, Make _check_suboptimal_rmse and _check_deterministic_policy_violation mutually exclusive by applying the RMSE suboptimality check only when the selection method is non-deterministic, while retaining the deterministic policy check for deterministic selections. Pass or otherwise use the selection method in _check_suboptimal_rmse and update all call sites, including the pre-check flag construction, so ratio violations produce exactly one model_selection critical flag.data_forecaster/backend/utils/statistical.py (1)
85-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the STL return type annotation
run_stl_decompositionnow returns astatus: stralongside the float lists, sodict[str, list[float]]is no longer accurate. Use aTypedDictordict[str, list[float] | str]so the mixed payload is reflected in the signature.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/utils/statistical.py` around lines 85 - 122, Update the return annotation of run_stl_decomposition to represent the mixed payload: trend, seasonal, and residual remain lists of floats, while status is a string. Use a suitable TypedDict or a union-valued dictionary annotation, preserving the existing return structure and behavior.Source: Coding guidelines
🟡 Minor comments (7)
data_forecaster/tests/test_report_renderers.py-281-361 (1)
281-361: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winXSS (CWE-79): Improper Neutralization of Input During Web Page Generation ('Cross-site Scripting')
Reachability: Unreachable
Enable autoescape in these Jinja tests
These rawjinja2.Environmentinstances bypass Flask’s default HTML autoescape, so the tests won’t catch escaping regressions inreport.htmlorforecast.html. Setautoescape=Truehere.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/tests/test_report_renderers.py` around lines 281 - 361, Enable HTML autoescaping on the raw Jinja Environment instances used by test_frontend_template_renders_interval_provenance_branches and test_forecast_template_treats_partial_bounds_as_unavailable by configuring each Environment with autoescape=True, while preserving the existing loaders, globals, and assertions.Source: Linters/SAST tools
tests/test_forecast_failure_states.py-183-186 (1)
183-186: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not mask serialization failures with
default=str.This fallback stringifies unsupported values, allowing the JSON-compatibility guarantee to pass falsely. Use
model_dump(mode="json")ormodel_dump_json()instead.Proposed fix
- data = result.model_dump() - json_str = json.dumps(data, default=str) + data = result.model_dump(mode="json") + json_str = json.dumps(data)Also applies to: 196-198
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_forecast_failure_states.py` around lines 183 - 186, Update the forecast failure-state tests using result.model_dump() to serialize through Pydantic’s JSON mode, such as model_dump(mode="json") or model_dump_json(), and remove json.dumps(..., default=str). Apply the same change to both affected assertions so unsupported values cause serialization failures instead of being stringified.data_forecaster/backend/forecasting/selection_policy.py-432-432 (1)
432-432: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMove in-function
import reto the module-level import block.Both
_check_invented_metricsand_check_contradictory_selectionimportrelocally instead of at the top of the file.As per coding guidelines, "Use full absolute imports, one import per line, grouped as future, standard library, third-party, and local imports."
Also applies to: 471-471
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/selection_policy.py` at line 432, Move the re import from the local scopes of _check_invented_metrics and _check_contradictory_selection into the module-level standard-library import block, keeping one import per line and removing both in-function imports.Source: Coding guidelines
data_forecaster/backend/forecasting/arima_model.py-21-39 (1)
21-39: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
_calculate_metricsdocstringArgs:doesn't match the actual signature.The docstring documents
trainandtestparameters, but the function actually takes(holdout, model, mase_period)—train/testdon't exist as parameters (they'reholdout.train/holdout.test), andmase_periodisn't documented.📝 Proposed fix
"""Calculate RMSE, MAE, and MAPE for the given model and test data. Args: - train: Training data used for MASE scale. - test: Holdout observations. + holdout: Terminal train/test holdout split. model: Trained ARIMA model with a ``predict`` method. + mase_period: Naive lag used for MASE scale estimation. Returns: Typed metrics. Unavailable evidence is never encoded as zero. """As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/arima_model.py` around lines 21 - 39, Update the Args section of _calculate_metrics to document the actual holdout, model, and mase_period parameters, describing holdout.train and holdout.test through the holdout argument; remove the nonexistent train and test entries.Source: Coding guidelines
data_forecaster/backend/forecasting/arima_model.py-110-159 (1)
110-159: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPass a concrete
with_interceptdefault
forecasting_agent.pyalready catches exceptions fromfit_arima, so the refit isn’t an uncaught crash path. The remaining issue iswith_intercept=Nonein thetrain_model is Nonebranch:pmdarima.ARIMAtreats that as falsy, so the fallback model silently drops the intercept instead of using the constructor default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/arima_model.py` around lines 110 - 159, Update the with_intercept fallback in the ARIMA refit block to pass a concrete boolean default when train_model is None, preserving the selected train_model.with_intercept value when available. Ensure the fallback uses the constructor’s intended intercept behavior rather than passing None into pm.ARIMA.data_forecaster/backend/schemas.py-215-238 (1)
215-238: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winKeep
n_missingon the same evaluation window as the other candidate metrics.candidate.metrics.n_missingcomes from the terminal holdout, while the other populated fields usebacktest_evals[name].pooled_metricswhen available. That mixes two different windows in one record; sourcen_missingfrom the backtest evaluation too, or rename the field to make the holdout provenance explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/schemas.py` around lines 215 - 238, Update the candidate result construction that populates ForecastCandidateResult so n_missing uses the same backtest_evals[name].pooled_metrics evaluation window as the other candidate metrics when available, rather than candidate.metrics.n_missing from the terminal holdout; preserve an appropriate fallback only when backtest metrics are unavailable.data_forecaster/backend/agents/statistical_review_agent.py-782-782 (1)
782-782: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
narrative_uncertaintycan be mislabeled "validated_llm_interpretation" even when validation never completed.
narrative_uncertaintyis set to"validated_llm_interpretation"right after_parse_verdict(output)(line 802), before_parse_flags,validate_llm_output, and_merge_review_flagsactually run. If any of those raise, theexceptblock (840-858) correctly falls back to deterministic-only flags/summary, butnarrative_uncertaintystill carries the stale"validated_llm_interpretation"value into the finalnarrative_claims— mislabeling the provenance of a claim that was actually built from the deterministic pre-check fallback only.Move the assignment to after the LLM-derived flags/validation have actually been merged.
🐛 Proposed fix
verdict = _parse_verdict(output) - narrative_uncertainty = "validated_llm_interpretation" llm_flags = _parse_flags(output) endorsements = _parse_endorsements(output) summary = _parse_summary(output) @@ all_flags = _merge_review_flags(pre_check_flags, llm_flags) + narrative_uncertainty = "validated_llm_interpretation"Also applies to: 802-825
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/statistical_review_agent.py` at line 782, Move the narrative_uncertainty assignment in the LLM review flow so "validated_llm_interpretation" is set only after _parse_flags, validate_llm_output, and _merge_review_flags complete successfully. Preserve "deterministic_precheck" in the exception fallback handled by the surrounding review logic, ensuring final narrative_claims reflect the actual validation provenance.
🧹 Nitpick comments (22)
data_forecaster/backend/forecasting/fixtures.py (1)
268-283: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
collections.abc.Callableinstead of the builtincallableas a type.
callableis a builtin function, not a type —dict[str, callable]won't be validated as intended by type checkers (mypy would flag it as not valid as a type). As per coding guidelines, "prefercollections.abctypes in signatures."♻️ Proposed fix
+from collections.abc import Callable + import numpy as np import pandas as pd ... -ALL_FIXTURES: dict[str, callable] = { +ALL_FIXTURES: dict[str, Callable[..., pd.Series]] = {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/fixtures.py` around lines 268 - 283, Update the ALL_FIXTURES annotation to use collections.abc.Callable instead of the builtin callable, adding the necessary import while preserving the existing dictionary structure and fixture mappings.Source: Coding guidelines
data_forecaster/backend/report/narrative.py (1)
173-173: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant
section.model_dump()call.
section.model_dump()is invoked once at line 173 (forsection_json) and again at line 188 (forsection_data) — same object, same output. Compute it once and reuse.♻️ Suggested fix
- section_json = json.dumps(section.model_dump(), default=str, indent=2) + section_data = section.model_dump() + section_json = json.dumps(section_data, default=str, indent=2) if extra_instructions: section_json += extra_instructions try: chain = prompt | llm inputs = {"section_json": section_json} response = chain.invoke(inputs) ... narrative = str(response.content).strip() - section_data = section.model_dump() valid_models = _models_in_evidence(section_data)Also applies to: 188-188
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/narrative.py` at line 173, Compute section.model_dump() once in the surrounding narrative-generation flow, store the result in a reusable variable, and use that variable for both section_json and section_data instead of invoking model_dump() again.data_forecaster/frontend/templates/main/forecast.html (1)
17-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove interval-availability logic out of the template.
forecast_rowsis already built inroutes.py'sforecast()handler (which already replaces out-of-range values withNone). Re-scanning it here in Jinja to derivebounds_available/interval_labelduplicates that logic in the presentation layer. As per coding guidelines, "Respect the architectural boundaries between backend/, frontend/, and core/", this determination reads better as backend logic passed into the template as a ready-madeinterval_label.♻️ Suggested direction
-{% set interval_state = namespace(label=fc.get('interval_label', 'prediction_interval'), bounds_available=(forecast_rows | length > 0)) %} -{% for row in forecast_rows %} - {% if row.lower_ci is none or row.upper_ci is none %} - {% set interval_state.bounds_available = false %} - {% endif %} -{% endfor %} -{% if not interval_state.bounds_available %} - {% set interval_state.label = 'unavailable' %} -{% endif %} -{% set interval_label = interval_state.label %} +{% set interval_label = interval_label %}And compute
interval_labelonce inroutes.py'sforecast()before rendering, passing it explicitly as a template variable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/frontend/templates/main/forecast.html` around lines 17 - 33, Move interval availability determination from the template into the routes.py forecast() handler, using the already-constructed forecast_rows and its None bounds to compute interval_label once before rendering. Pass the resulting interval_label explicitly to the template, then remove the Jinja namespace, row scan, and fallback-label logic while preserving the existing unavailable, experimental, and default message rendering.Source: Coding guidelines
data_forecaster/backend/agents/statistical_analysis_agent.py (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Args/Returns to
run_statistical_agent's docstring.This is a complex, central, public function (4 params, ~100 lines) but has only a one-line docstring. As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/statistical_analysis_agent.py` around lines 41 - 47, Expand the docstring for run_statistical_agent to Google-style documentation, adding an Args section describing series, seasonal_period, user_domain, and disabled_tests, plus a Returns section describing the StatisticalResult produced. Preserve the existing summary and document any relevant defaults or optional behavior.Source: Coding guidelines
data_forecaster/backend/rag/knowledge_base.py (1)
14-14: 🗄️ Data Integrity & Integration | 🔵 TrivialConfigurable
EMBED_MODELrisks stale/mismatched Chroma collections.
get_or_create_collectionwill silently reuse a persisted collection built with a different (previous)EMBED_MODEL. If the config value changes after initial ingestion,upsertat Line 85 will either fail (dimension mismatch) or silently mix incompatible embeddings in the same collection. Consider namespacing the collection name (or storing the embedding model in collection metadata and validating it) so a model change forces a fresh collection/re-embed rather than corrupting an existing one.Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/rag/knowledge_base.py` at line 14, Update the collection setup in get_or_create_collection and its call site near the upsert flow to prevent reuse across different EMBED_MODEL values. Namespace the persisted collection name with EMBED_MODEL, or store and validate the model in collection metadata, ensuring a model change selects a fresh collection and requires re-embedding.data_forecaster/backend/forecasting/diagnostics.py (1)
761-832: 🚀 Performance & Scalability | 🔵 TrivialRecursive change-point detection re-runs 100-permutation calibration at every split.
Each recursive call to
_binary_segmentationtriggers a fresh 100-permutation_calibrate_thresholdon its segment; for series with many true change points this compounds close to O(permutations × n²). Worth confirming this stays performant for the largest series lengths this pipeline processes (e.g., long daily series), since diagnostics may run repeatedly across rolling-origin folds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/diagnostics.py` around lines 761 - 832, Update _binary_segmentation and _calibrate_threshold to avoid recalibrating 100 permutations independently for every recursive segment. Reuse a single calibration result or otherwise share permutation-derived threshold data across recursive splits while preserving the existing detection behavior and threshold semantics. Ensure the change remains efficient for long series and repeated diagnostics.data_forecaster/backend/utils/preflight.py (1)
88-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated “None known” value to clear the SonarCloud finding.
Use a module-level constant for the shared default and option value.
Also applies to: 166-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/utils/preflight.py` around lines 88 - 90, Define a module-level constant for the shared “None known” value in preflight configuration, then replace both the default and option entries for “interventions” and “censoring_or_stockouts” with that constant. Preserve the existing serialized value and behavior.Source: Linters/SAST tools
data_forecaster/backend/forecasting/sarima_model.py (2)
21-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale docstring
Argsfor_calculate_metrics.Docstring lists
train,test,model,seasonal_period, but the signature is(holdout, model, mase_period).As per coding guidelines, "Use ... Google-style docstrings for public, nontrivial, or non-obvious functions."
📝 Proposed fix
Args: - train: Training data used for MASE scale. - test: Holdout observations. - model: Trained SARIMA model with a ``predict`` method. - seasonal_period: Seasonal period used for the MASE naive lag. + holdout: Terminal train/test split used for evidence scoring. + model: Trained SARIMA model with a ``predict`` method. + mase_period: Seasonal period used for the MASE naive lag.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/sarima_model.py` around lines 21 - 32, Update the `_calculate_metrics` docstring `Args` section to document the actual parameters `holdout`, `model`, and `mase_period`, removing the stale `train`, `test`, and `seasonal_period` entries while preserving the existing return documentation.Source: Coding guidelines
47-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSonarCloud:
fit_sarimacognitive complexity 21/15.Consider extracting the post-fit diagnostics/warnings assembly (147-189) into a helper, mirroring the
_calculate_metricsextraction already done.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/sarima_model.py` around lines 47 - 52, Reduce cognitive complexity in fit_sarima by extracting the post-fit diagnostics and warnings assembly currently spanning the indicated block into a dedicated helper, following the existing _calculate_metrics pattern. Update fit_sarima to call the helper while preserving the current diagnostics, warning contents, and returned ForecastAdapterResult behavior.Source: Linters/SAST tools
data_forecaster/backend/agents/forecasting_agent.py (3)
1014-1019: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSonarCloud:
_run_residual_diagnosticscognitive complexity 17/15.Close to the limit; extracting the fold-to-arrays assembly loop (1032-1045) into a small helper would bring it back under threshold.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/forecasting_agent.py` around lines 1014 - 1019, Reduce the cognitive complexity of _run_residual_diagnostics by extracting its fold-to-arrays assembly loop into a focused helper that performs the same conversion and collection. Replace the inline loop with the helper result, preserving existing ordering, filtering, and diagnostic behavior.Source: Linters/SAST tools
713-722: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSonarCloud:
_run_backtest_evaluationcognitive complexity 35/15.The eight nested candidate closures (
_arima_fn…_drift_fn, plus_transformed_candidate) share a common try/log/return-None-on-failure shape; consider extracting a shared wrapper/decorator to cut branching in the outer function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/forecasting_agent.py` around lines 713 - 722, Reduce cognitive complexity in _run_backtest_evaluation by extracting the repeated try/log/return-None-on-failure behavior from the candidate closures (_arima_fn through _drift_fn and _transformed_candidate) into a shared wrapper or decorator. Update each candidate to use that helper while preserving its existing forecasting logic, logging, and failure behavior.Source: Linters/SAST tools
638-638: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSonarCloud: duplicate literals
"Seasonal Naive"/"Mean Forecast"(4× each).
_BASELINE_NAMESalready centralizes the baseline names as a set; extract per-name constants and reuse them at eachfitted_configuration/candidates-dict site (653/656, 909/917, 977/978) instead of re-typing the strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/agents/forecasting_agent.py` at line 638, Extract individual constants for “Seasonal Naive” and “Mean Forecast” from _BASELINE_NAMES, then reuse those constants at every fitted_configuration and candidates-dictionary site identified in the forecasting agent, including the usages around 653/656, 909/917, and 977/978. Preserve _BASELINE_NAMES membership while eliminating all repeated literal strings.Source: Linters/SAST tools
data_forecaster/backend/forecasting/residual_diagnostics.py (3)
257-257: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
zip()withoutstrict=on fold-aligned lists.
fold_actuals/fold_lower/fold_upperare built in lockstep by the caller, so a length mismatch would indicate a real bug upstream.strict=Trueturns silent truncation into an explicit error.Also applies to: 366-366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/residual_diagnostics.py` at line 257, Update the fold-aligned iteration loops in the residual diagnostics code, including the loop over fold_actuals, fold_lower, and fold_upper and the corresponding loop near the second referenced location, to call zip with strict=True. Preserve the existing loop bodies and behavior while making any length mismatch raise an explicit error instead of silently truncating.Source: Linters/SAST tools
278-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSonarCloud:
analyze_backtest_errorscognitive complexity 30/15.Consider extracting the per-horizon coverage/width/Winkler loop (362-389) into a helper alongside
_compute_interval_metrics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/residual_diagnostics.py` around lines 278 - 286, Reduce cognitive complexity in analyze_backtest_errors by extracting the per-horizon coverage, interval-width, and Winkler-score loop into a dedicated helper near _compute_interval_metrics. Have the helper return the aggregated metrics needed by analyze_backtest_errors, preserving existing handling of fold_lower, fold_upper, nominal_coverage, and missing intervals.Source: Linters/SAST tools
43-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the custom df fallback if you switch to
model_df.acorr_ljungboxin statsmodels 0.14.0 does supportmodel_df, but it returnsNaNwhenlag <= model_df; this code currently clamps the effective df to at least 1, so a direct swap would change small-sample behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/residual_diagnostics.py` around lines 43 - 80, Update _ljung_box to pass df_adjust through acorr_ljungbox’s model_df parameter, while retaining the existing custom chi-square fallback with effective_df clamped to at least 1 when lag <= df_adjust. Preserve the current finite p-value behavior and lag handling for small samples.data_forecaster/backend/report/builder.py (1)
654-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLocal variable shadows the imported
confidence_labelfunction.
confidence_labelis imported fromreport.rulesas a function (used in_compute_confidence). Reassigning it as a local string here works today only because the function isn't called later in this method, but it's a latent trap for future edits in this scope.♻️ Proposed rename
- confidence_label = ( + confidence_level_label = ( "95% (experimental)" if interval_label == "experimental" else _CONFIDENCE_LEVEL )(and update the corresponding
confidence_level=confidence_labelreference below toconfidence_level_label.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/builder.py` around lines 654 - 665, Rename the local string variable in the report-building method from confidence_label to confidence_level_label to avoid shadowing the imported confidence_label function, and update the corresponding confidence_level assignment below to use the renamed variable. Leave the imported function and _compute_confidence usage unchanged.data_forecaster/backend/report/dashboard.py (2)
188-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate pooled/final RMSE extraction logic.
This block reimplements the same "selection_metrics rmse, fallback to forecast.rmse, compute ratio" logic already encapsulated in
builder.py's_recent_holdout_rmse_ratio(forecast). Sincebuilder.pyimports fromdashboard.py(risk of circular import if reversed), consider moving this extraction intoreport/rules.pyas a sharedforecast_holdout_ratio(forecast)helper that both modules call, to avoid the two implementations drifting apart.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/dashboard.py` around lines 188 - 196, The forecast holdout-ratio calculation is duplicated between the dashboard flow and builder logic. Add a shared forecast_holdout_ratio(forecast) helper in report/rules.py that performs pooled RMSE selection, fallback to forecast.rmse, and final-test ratio computation; update the dashboard block and builder’s _recent_holdout_rmse_ratio to call it while avoiding a dashboard↔builder circular import.
112-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
direction_statushelperdata_forecaster/backend/report/dashboard.py:112-118no longer has any callers, so this can be deleted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/dashboard.py` around lines 112 - 118, Remove the unused direction_status helper, including its docstring and conditional logic, from the dashboard module. Do not alter the surrounding forecast direction constants or any other reporting behavior.data_forecaster/backend/report/rules.py (1)
52-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd full Args/Returns docstring for
recent_holdout_rmse_ratio.This is a public function with non-obvious
object-typed parameters; a one-line summary doesn't cover their meaning or theNonereturn case.📝 Proposed docstring
def recent_holdout_rmse_ratio( final_test_rmse: object, pooled_rolling_rmse: object, ) -> float | None: - """Return the latest-holdout/rolling-origin RMSE ratio when valid.""" + """Return the latest-holdout/rolling-origin RMSE ratio when valid. + + Args: + final_test_rmse: Untouched final-test RMSE, or ``None``/invalid. + pooled_rolling_rmse: Pooled rolling-origin RMSE, or ``None``/invalid. + + Returns: + The ratio as a float, or ``None`` when either input is a bool, + non-numeric, non-finite, or the denominator is non-positive. + """As per coding guidelines, "Use module-level docstrings and Google-style docstrings for public, nontrivial, or non-obvious functions, methods, and classes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/report/rules.py` around lines 52 - 69, Expand the docstring for recent_holdout_rmse_ratio to Google-style documentation, describing final_test_rmse and pooled_rolling_rmse as numeric RMSE inputs, the validation conditions that produce None, and the returned latest-holdout-to-rolling-origin ratio when valid.Source: Coding guidelines
data_forecaster/backend/schemas.py (1)
174-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a
Literaltype forselection_methodinstead of a barestr+ comment.The comment documents 4 valid values (
"deterministic" | "llm" | "heuristic" | "forced"), but nothing enforces them. Since this field drives report/dashboard narrative branching (perreport/builder.py's_selection_rationale), a stricter type would catch typos at validation time.- selection_method: str = "llm" # "deterministic" | "llm" | "heuristic" | "forced" + selection_method: Literal["deterministic", "llm", "heuristic", "forced"] = "llm"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/schemas.py` around lines 174 - 177, Update the selection_method field in the relevant schema to use a Literal type restricted to "deterministic", "llm", "heuristic", and "forced", removing reliance on the descriptive comment while preserving the existing default of "llm".data_forecaster/backend/forecasting/ewma_model.py (1)
103-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
_estimate_alpha's grid-search result is discarded wheneveralphaisNoneand the fit succeeds.At line 107,
estimated_alpha = _estimate_alpha(train)whenalpha is None. But at line 111-114, sincealpha is Noneimpliesoptimized=Truewithsmoothing_level=None, statsmodels performs its own independent MLE optimization and ignores the grid-search value entirely —estimated_alphais then unconditionally overwritten fromtrain_fit.params["smoothing_level"]. So the grid search only ever influences the final model when thetryblock raises an exception. This contradicts the module/function docstrings ("The adapter estimatesalphaby minimizing one-step-ahead squared error on the training split") and wastes ~99.ewm()passes overtrainon the common path.Either drop
_estimate_alphafrom the primary path (only compute it in theexceptfallback) or passsmoothing_level=estimated_alpha, optimized=Falsetotrain_fitto make the grid search authoritative — please confirm which is the intended design.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@data_forecaster/backend/forecasting/ewma_model.py` around lines 103 - 124, The EWMA training flow computes _estimate_alpha but discards it when fitting succeeds. Make the grid-search estimate authoritative by passing estimated_alpha as the fit’s smoothing_level and disabling optimizer-driven replacement in SimpleExpSmoothing, while preserving the explicit-alpha path and existing metrics/error handling.tests/test_model_retry_consistency.py (1)
52-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
@pytest.fixtureinstead of plain factory functions.
_statistical_result()and_forecast()are reused across multiple tests as fixed test data; per repo test conventions these are good candidates forpytest.fixture-based fixtures rather than plain helper functions.As per path instructions, "Use pytest fixtures, mock external HTTP and LLM calls, use
monkeypatchfor environment changes, and useTestClientfor FastAPI integration tests" fortests/**/*.py.Also applies to: 67-80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_model_retry_consistency.py` around lines 52 - 64, Convert the shared _statistical_result() and _forecast() test-data helpers into pytest fixtures, preserving their existing returned values and updating all test call sites to request the fixtures as parameters instead of invoking functions. Follow the repository’s fixture conventions and keep the test behavior unchanged.Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 517069bb-27d3-41e6-824c-9a0ea9e2adfe
📒 Files selected for processing (61)
data_forecaster/backend/.env.exampledata_forecaster/backend/agents/data_validation_agent.pydata_forecaster/backend/agents/forecasting_agent.pydata_forecaster/backend/agents/model_selection_agent.pydata_forecaster/backend/agents/report_generation_agent.pydata_forecaster/backend/agents/statistical_analysis_agent.pydata_forecaster/backend/agents/statistical_review_agent.pydata_forecaster/backend/core/config.pydata_forecaster/backend/forecasting/arima_model.pydata_forecaster/backend/forecasting/backtesting.pydata_forecaster/backend/forecasting/contracts.pydata_forecaster/backend/forecasting/diagnostics.pydata_forecaster/backend/forecasting/evaluation.pydata_forecaster/backend/forecasting/ewma_model.pydata_forecaster/backend/forecasting/fixtures.pydata_forecaster/backend/forecasting/holt_winters.pydata_forecaster/backend/forecasting/metrics.pydata_forecaster/backend/forecasting/preprocessing.pydata_forecaster/backend/forecasting/residual_diagnostics.pydata_forecaster/backend/forecasting/sarima_model.pydata_forecaster/backend/forecasting/selection_policy.pydata_forecaster/backend/prompts/forecasting_prompt.pydata_forecaster/backend/prompts/general_chat_prompt.pydata_forecaster/backend/prompts/model_selection_prompt.pydata_forecaster/backend/prompts/orchestrator_prompt.pydata_forecaster/backend/prompts/report_generation_prompt.pydata_forecaster/backend/prompts/statistical_analysis_prompt.pydata_forecaster/backend/prompts/statistical_review_prompt.pydata_forecaster/backend/rag/knowledge_base.pydata_forecaster/backend/report/builder.pydata_forecaster/backend/report/dashboard.pydata_forecaster/backend/report/models.pydata_forecaster/backend/report/narrative.pydata_forecaster/backend/report/renderers/html_renderer.pydata_forecaster/backend/report/renderers/markdown_renderer.pydata_forecaster/backend/report/rules.pydata_forecaster/backend/schemas.pydata_forecaster/backend/services/baseline_service.pydata_forecaster/backend/services/pipeline_service.pydata_forecaster/backend/utils/data_cleaning.pydata_forecaster/backend/utils/preflight.pydata_forecaster/backend/utils/statistical.pydata_forecaster/backend/utils/validation.pydata_forecaster/backend/utils/visualization.pydata_forecaster/docker/Dockerfile.flaskdata_forecaster/frontend/blueprints/main/routes.pydata_forecaster/frontend/services/pdf_service.pydata_forecaster/frontend/static/js/app.jsdata_forecaster/frontend/templates/main/forecast.htmldata_forecaster/frontend/templates/main/report.htmldata_forecaster/frontend/templates/main/started.htmldata_forecaster/tests/test_report_builder.pydata_forecaster/tests/test_report_renderers.pydata_forecaster/tests/test_report_rules.pytests/test_airline_report_consistency.pytests/test_decision_loss.pytests/test_forecast_failure_states.pytests/test_forecast_fixtures.pytests/test_forecasting_metrics.pytests/test_model_retry_consistency.pytests/test_pdf_service.py
| # Deterministic policy: lowest RMSE wins. The LLM never decides | ||
| # model rankings. | ||
| selected = min( | ||
| rankable, key=lambda name: rankable[name].metrics.rmse or float("inf") | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Falsy-zero bug: a perfect-fit model (RMSE=0.0) is ranked as worst.
rankable is already filtered to entries with non-None, finite RMSE via is_rankable, so or float("inf") is unnecessary — and actively wrong when metrics.rmse == 0.0, since 0.0 is falsy in Python and gets replaced by inf, making the best model look like the worst.
🐛 Proposed fix
- selected = min(
- rankable, key=lambda name: rankable[name].metrics.rmse or float("inf")
- )
+ selected = min(rankable, key=lambda name: rankable[name].metrics.rmse)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Deterministic policy: lowest RMSE wins. The LLM never decides | |
| # model rankings. | |
| selected = min( | |
| rankable, key=lambda name: rankable[name].metrics.rmse or float("inf") | |
| ) | |
| # Deterministic policy: lowest RMSE wins. The LLM never decides | |
| # model rankings. | |
| selected = min(rankable, key=lambda name: rankable[name].metrics.rmse) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/agents/forecasting_agent.py` around lines 416 - 420,
Update the selection key in the deterministic ranking logic around selected and
rankable to use each model’s validated metrics.rmse directly, removing the `or
float("inf")` fallback so an RMSE of 0.0 remains the best score.
| def evaluate_candidate( | ||
| name: str, | ||
| series: pd.Series, | ||
| folds: Sequence[BacktestFold], | ||
| candidate_fn: CandidateFn, | ||
| config: BacktestConfig, | ||
| ) -> BacktestEvaluation: | ||
| """Evaluate one candidate model across all folds. | ||
|
|
||
| Args: | ||
| name: Candidate model name. | ||
| series: Full cleaned time series. | ||
| folds: Fold definitions shared by all candidates. | ||
| candidate_fn: Callable that fits on the fold training window and | ||
| returns predictions for the fold test window. | ||
| config: Backtesting configuration (used for ``mase_period``). | ||
|
|
||
| Returns: | ||
| :class:`BacktestEvaluation` with per-fold results and pooled metrics. | ||
| """ | ||
| fold_results: list[BacktestFoldResult] = [] | ||
| pooled_actuals: list[float] = [] | ||
| pooled_preds: list[float] = [] | ||
| by_horizon_actuals: dict[int, list[float]] = {} | ||
| by_horizon_preds: dict[int, list[float]] = {} | ||
| warnings: list[str] = [] | ||
|
|
||
| for fold in folds: | ||
| result = _process_fold( | ||
| name, | ||
| series, | ||
| fold, | ||
| candidate_fn, | ||
| pooled_actuals, | ||
| pooled_preds, | ||
| by_horizon_actuals, | ||
| by_horizon_preds, | ||
| warnings, | ||
| config, | ||
| ) | ||
| if result is not None: | ||
| fold_results.append(result) | ||
|
|
||
| if folds: | ||
| strategy = "clip" if config.apply_iqr_clip else config.outlier_strategy | ||
| initial_series = prepare_training_series( | ||
| series.iloc[: folds[0].train_end_index].copy(), | ||
| outlier_strategy=strategy, | ||
| imputation_method=config.imputation_method, | ||
| smoothing_method=config.smoothing_method, | ||
| ) | ||
| initial_training = initial_series.to_numpy(dtype=float) | ||
| else: | ||
| initial_training = np.asarray([], dtype=float) | ||
| pooled = calculate_forecast_metrics( | ||
| np.asarray(pooled_actuals, dtype=float), | ||
| np.asarray(pooled_preds, dtype=float), | ||
| training=initial_training, | ||
| mase_period=config.mase_period, | ||
| ) | ||
|
|
||
| by_horizon: dict[int, ForecastMetrics] = {} | ||
| for h in sorted(by_horizon_actuals): | ||
| by_horizon[h] = calculate_forecast_metrics( | ||
| np.asarray(by_horizon_actuals[h], dtype=float), | ||
| np.asarray(by_horizon_preds[h], dtype=float), | ||
| training=initial_training, | ||
| mase_period=config.mase_period, | ||
| ) | ||
|
|
||
| n_evaluated = pooled.n_evaluated | ||
| unavailable = dict(pooled.unavailable_reasons) | ||
| if not fold_results: | ||
| unavailable.setdefault("all", "No folds were evaluated.") | ||
|
|
||
| successful_origins = sum( | ||
| fold.status == ForecastFitStatus.OK for fold in fold_results | ||
| ) | ||
| final_test_metrics = ForecastMetrics( | ||
| unavailable_reasons={"all": "No untouched final test window was reserved."} | ||
| ) | ||
| if config.final_test_size > 0 and len(series) > config.final_test_size: | ||
| final_start = len(series) - config.final_test_size | ||
| final_fold = BacktestFold( | ||
| fold_index=len(folds), | ||
| train_end_index=final_start, | ||
| test_start_index=final_start, | ||
| test_end_index=len(series), | ||
| horizon=config.final_test_size, | ||
| ) | ||
| final_actuals: list[float] = [] | ||
| final_predictions: list[float] = [] | ||
| final_result = _process_fold( | ||
| name, | ||
| series, | ||
| final_fold, | ||
| candidate_fn, | ||
| final_actuals, | ||
| final_predictions, | ||
| {}, | ||
| {}, | ||
| warnings, | ||
| config, | ||
| ) | ||
| if final_result is not None and final_result.status == ForecastFitStatus.OK: | ||
| strategy = "clip" if config.apply_iqr_clip else config.outlier_strategy | ||
| final_training = prepare_training_series( | ||
| series.iloc[:final_start].copy(), | ||
| outlier_strategy=strategy, | ||
| imputation_method=config.imputation_method, | ||
| smoothing_method=config.smoothing_method, | ||
| ) | ||
| final_test_metrics = calculate_forecast_metrics( | ||
| np.asarray(final_actuals, dtype=float), | ||
| np.asarray(final_predictions, dtype=float), | ||
| training=final_training, | ||
| mase_period=config.mase_period, | ||
| ) | ||
| else: | ||
| final_test_metrics = ForecastMetrics( | ||
| unavailable_reasons={ | ||
| "all": "Candidate failed on the untouched final test window." | ||
| } | ||
| ) | ||
| evaluated_horizon = folds[0].horizon if folds else 0 | ||
| requested_horizon = config.requested_horizon or config.horizon or evaluated_horizon | ||
| return BacktestEvaluation( | ||
| model_name=name, | ||
| folds=fold_results, | ||
| pooled_metrics=pooled, | ||
| final_test_metrics=final_test_metrics, | ||
| by_horizon_metrics=by_horizon, | ||
| n_origins=successful_origins, | ||
| n_failed_origins=len(fold_results) - successful_origins, | ||
| n_evaluated=n_evaluated, | ||
| validation_design={ | ||
| "method": "expanding_window", | ||
| "initial_train_size": folds[0].train_end_index if folds else 0, | ||
| "requested_horizon": requested_horizon, | ||
| "evaluated_horizon": evaluated_horizon, | ||
| "unsupported_horizons": list( | ||
| range(evaluated_horizon + 1, requested_horizon + 1) | ||
| ), | ||
| "step_size": config.step_size or evaluated_horizon, | ||
| "gap": config.gap, | ||
| "max_origins": config.max_origins, | ||
| "successful_origins": successful_origins, | ||
| "failed_origins": len(fold_results) - successful_origins, | ||
| "n_evaluated": n_evaluated, | ||
| "mase_period": config.mase_period, | ||
| "apply_iqr_clip": config.apply_iqr_clip, | ||
| "outlier_strategy": config.outlier_strategy, | ||
| "imputation_method": config.imputation_method, | ||
| "smoothing_method": config.smoothing_method, | ||
| "final_test_size": config.final_test_size, | ||
| "selection_end_index": len(series) - config.final_test_size, | ||
| }, | ||
| metric_intervals=_bootstrap_metric_intervals( | ||
| np.asarray(pooled_actuals, dtype=float), | ||
| np.asarray(pooled_preds, dtype=float), | ||
| initial_training, | ||
| config.mase_period, | ||
| ), | ||
| unavailable_reasons=unavailable, | ||
| warnings=warnings, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cognitive complexity CI check fails on evaluate_candidate.
SonarCloud reports cognitive complexity 22 vs. the allowed 15 (CI failure). The final-test-window block (394-436) and the validation_design dict construction (448-469) are good candidates to extract into helpers.
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 313-313: Refactor this function to reduce its Cognitive Complexity from 22 to the 15 allowed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/forecasting/backtesting.py` around lines 313 - 478,
Reduce cognitive complexity in evaluate_candidate below the CI threshold by
extracting the final-test-window evaluation into a focused helper and moving
validation_design construction into another helper. Update evaluate_candidate to
call these helpers while preserving all existing metrics, failure handling, and
validation metadata.
Source: Linters/SAST tools
| status = ( | ||
| ForecastFitStatus.OK if metrics.rmse is not None else ForecastFitStatus.DEGRADED | ||
| ) | ||
| failure_reason = ( | ||
| None if metrics.rmse is not None else metrics.unavailable_reasons.get("all") | ||
| ) | ||
| rmse = metrics.get("rmse") | ||
| mae = metrics.get("mae") | ||
| mape = metrics.get("mape") | ||
| if not metrics: | ||
| logger.warning("EWMA rolling validation failed; metrics unavailable.") | ||
|
|
||
| # ── Full-series fit for forecast ───────────────────────────────────────── | ||
| # Calculate EWMA for entire series | ||
| full_ewma = series.ewm(alpha=alpha).mean() | ||
| last_full_value = full_ewma.iloc[-1] | ||
|
|
||
| # Forecast: use the last EWMA value for all future periods | ||
| forecast_values = [last_full_value] * forecast_horizon | ||
|
|
||
| # Calculate confidence intervals using rolling standard deviation | ||
| residuals = series - full_ewma | ||
| std_residuals = np.std(residuals.dropna()) | ||
|
|
||
| # 95% confidence intervals (approximate) | ||
| lower_ci = [f - 1.96 * std_residuals for f in forecast_values] | ||
| upper_ci = [f + 1.96 * std_residuals for f in forecast_values] | ||
|
|
||
| logger.info("EWMA model fitted with alpha=%.2f", alpha) | ||
|
|
||
| return { | ||
| "forecast": forecast_values, | ||
| "lower_ci": lower_ci, | ||
| "upper_ci": upper_ci, | ||
| "rmse": rmse, | ||
| "mae": mae, | ||
| "mape": mape, | ||
| } | ||
| return ForecastAdapterResult( | ||
| status=status, | ||
| failure_reason=failure_reason, | ||
| is_fallback=False, | ||
| forecast=forecast_values.tolist(), | ||
| lower_ci=lower_ci, | ||
| upper_ci=upper_ci, | ||
| metrics=metrics, | ||
| fitted_configuration={ | ||
| "model": "EWMA", | ||
| "alpha": estimated_alpha, | ||
| "initialization": "level", | ||
| "estimated": alpha is None, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
is_fallback is hardcoded False even when the training-split fit fails.
If the try block at lines 110-123 raises, metrics becomes unavailable and estimated_alpha falls back to _estimate_alpha's grid-search value (not a data-driven optimized alpha), yet the returned result still sets is_fallback=False. Per ForecastAdapterResult's contract, is_fallback should reflect "whether this result is a fallback/persistence forecast" — arima_model.py's fit_arima correctly sets is_fallback=train_model is None for the analogous scenario. EWMA should track whether the training-split estimation actually succeeded and reflect that here.
🐛 Proposed fix
+ training_fit_succeeded = False
try:
train_fit = SimpleExpSmoothing(
train, initialization_method="estimated"
).fit(smoothing_level=alpha, optimized=alpha is None)
estimated_alpha = float(train_fit.params["smoothing_level"])
+ training_fit_succeeded = True
test_fc = np.asarray(train_fit.forecast(len(test)), dtype=float)
metrics = evaluate_predictions(
holdout,
test_fc,
mase_period=mase_period,
)
except Exception as exc: # pylint: disable=broad-except
logger.warning("EWMA metrics calculation failed: %s", exc)
metrics = ForecastMetrics(unavailable_reasons={"all": str(exc)})
...
return ForecastAdapterResult(
status=status,
failure_reason=failure_reason,
- is_fallback=False,
+ is_fallback=not training_fit_succeeded,As per coding guidelines, "Respect the architectural boundaries... and scan similar files for established patterns before introducing new ones" — arima_model.py already establishes the is_fallback=<training succeeded> pattern that EWMA should follow.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status = ( | |
| ForecastFitStatus.OK if metrics.rmse is not None else ForecastFitStatus.DEGRADED | |
| ) | |
| failure_reason = ( | |
| None if metrics.rmse is not None else metrics.unavailable_reasons.get("all") | |
| ) | |
| rmse = metrics.get("rmse") | |
| mae = metrics.get("mae") | |
| mape = metrics.get("mape") | |
| if not metrics: | |
| logger.warning("EWMA rolling validation failed; metrics unavailable.") | |
| # ── Full-series fit for forecast ───────────────────────────────────────── | |
| # Calculate EWMA for entire series | |
| full_ewma = series.ewm(alpha=alpha).mean() | |
| last_full_value = full_ewma.iloc[-1] | |
| # Forecast: use the last EWMA value for all future periods | |
| forecast_values = [last_full_value] * forecast_horizon | |
| # Calculate confidence intervals using rolling standard deviation | |
| residuals = series - full_ewma | |
| std_residuals = np.std(residuals.dropna()) | |
| # 95% confidence intervals (approximate) | |
| lower_ci = [f - 1.96 * std_residuals for f in forecast_values] | |
| upper_ci = [f + 1.96 * std_residuals for f in forecast_values] | |
| logger.info("EWMA model fitted with alpha=%.2f", alpha) | |
| return { | |
| "forecast": forecast_values, | |
| "lower_ci": lower_ci, | |
| "upper_ci": upper_ci, | |
| "rmse": rmse, | |
| "mae": mae, | |
| "mape": mape, | |
| } | |
| return ForecastAdapterResult( | |
| status=status, | |
| failure_reason=failure_reason, | |
| is_fallback=False, | |
| forecast=forecast_values.tolist(), | |
| lower_ci=lower_ci, | |
| upper_ci=upper_ci, | |
| metrics=metrics, | |
| fitted_configuration={ | |
| "model": "EWMA", | |
| "alpha": estimated_alpha, | |
| "initialization": "level", | |
| "estimated": alpha is None, | |
| }, | |
| training_fit_succeeded = False | |
| try: | |
| train_fit = SimpleExpSmoothing( | |
| train, initialization_method="estimated" | |
| ).fit(smoothing_level=alpha, optimized=alpha is None) | |
| estimated_alpha = float(train_fit.params["smoothing_level"]) | |
| training_fit_succeeded = True | |
| test_fc = np.asarray(train_fit.forecast(len(test)), dtype=float) | |
| metrics = evaluate_predictions( | |
| holdout, | |
| test_fc, | |
| mase_period=mase_period, | |
| ) | |
| except Exception as exc: # pylint: disable=broad-except | |
| logger.warning("EWMA metrics calculation failed: %s", exc) | |
| metrics = ForecastMetrics(unavailable_reasons={"all": str(exc)}) | |
| status = ( | |
| ForecastFitStatus.OK if metrics.rmse is not None else ForecastFitStatus.DEGRADED | |
| ) | |
| failure_reason = ( | |
| None if metrics.rmse is not None else metrics.unavailable_reasons.get("all") | |
| ) | |
| return ForecastAdapterResult( | |
| status=status, | |
| failure_reason=failure_reason, | |
| is_fallback=not training_fit_succeeded, | |
| forecast=forecast_values.tolist(), | |
| lower_ci=lower_ci, | |
| upper_ci=upper_ci, | |
| metrics=metrics, | |
| fitted_configuration={ | |
| "model": "EWMA", | |
| "alpha": estimated_alpha, | |
| "initialization": "level", | |
| "estimated": alpha is None, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/forecasting/ewma_model.py` around lines 151 - 171,
Update the EWMA training flow around the try/except that produces
estimated_alpha and metrics to track whether training-split estimation
succeeded, following the established fit_arima pattern in arima_model.py. Set
ForecastAdapterResult.is_fallback from that success state instead of hardcoding
False, while preserving the existing fallback alpha and forecast behavior when
estimation fails.
Source: Coding guidelines
| def calculate_forecast_metrics( | ||
| actual: np.ndarray | pd.Series, | ||
| predicted: np.ndarray | pd.Series, | ||
| *, | ||
| training: np.ndarray | pd.Series | None = None, | ||
| mase_period: int = 1, | ||
| ) -> ForecastMetrics: | ||
| """Calculate point metrics under one documented set of conventions. | ||
|
|
||
| MAPE is unavailable when actuals contain zeros. MASE uses a fixed naive | ||
| lag supplied by the caller and is unavailable when its scale cannot be | ||
| estimated. WAPE uses the sum of absolute actuals as its denominator. | ||
| """ | ||
| y_true = np.asarray(actual, dtype=float) | ||
| y_pred = np.asarray(predicted, dtype=float) | ||
| if y_true.shape != y_pred.shape or y_true.size == 0: | ||
| return ForecastMetrics( | ||
| unavailable_reasons={ | ||
| "all": "Actual and predicted values must be non-empty and aligned." | ||
| } | ||
| ) | ||
| finite = np.isfinite(y_true) & np.isfinite(y_pred) | ||
| n_missing = int(y_true.size - np.count_nonzero(finite)) | ||
| y_true = y_true[finite] | ||
| y_pred = y_pred[finite] | ||
| if y_true.size == 0: | ||
| return ForecastMetrics( | ||
| unavailable_reasons={"all": "No finite aligned observations."} | ||
| ) | ||
|
|
||
| errors = y_true - y_pred | ||
| absolute_errors = np.abs(errors) | ||
| reasons: dict[str, str] = {} | ||
| mape = None | ||
| if np.any(y_true == 0): | ||
| reasons["mape"] = "MAPE is undefined when any actual value is zero." | ||
| else: | ||
| mape = float(np.mean(np.abs(errors / y_true)) * 100) | ||
|
|
||
| denominator = float(np.sum(np.abs(y_true))) | ||
| wape = None | ||
| if denominator == 0: | ||
| reasons["wape"] = ( | ||
| "WAPE is undefined when the absolute-actual denominator is zero." | ||
| ) | ||
| else: | ||
| wape = float(np.sum(absolute_errors) / denominator) | ||
|
|
||
| mase = None | ||
| rmsse = None | ||
| if training is None: | ||
| reasons["mase"] = "Training data is required for MASE." | ||
| reasons["rmsse"] = "Training data is required for RMSSE." | ||
| else: | ||
| train = np.asarray(training, dtype=float) | ||
| train = train[np.isfinite(train)] | ||
| if mase_period < 1 or train.size <= mase_period: | ||
| reasons["mase"] = "Training data is too short for the configured naive lag." | ||
| reasons["rmsse"] = ( | ||
| "Training data is too short for the configured naive lag." | ||
| ) | ||
| else: | ||
| scale = float(np.mean(np.abs(train[mase_period:] - train[:-mase_period]))) | ||
| if scale == 0: | ||
| reasons["mase"] = ( | ||
| "MASE is undefined because the naive error scale is zero." | ||
| ) | ||
| reasons["rmsse"] = ( | ||
| "RMSSE is undefined because the naive squared-error scale is zero." | ||
| ) | ||
| else: | ||
| mase = float(np.mean(absolute_errors) / scale) | ||
| squared_scale = float( | ||
| np.mean((train[mase_period:] - train[:-mase_period]) ** 2) | ||
| ) | ||
| if squared_scale > 0: | ||
| rmsse = float(np.sqrt(np.mean(errors**2) / squared_scale)) | ||
| smape_denominator = np.abs(y_true) + np.abs(y_pred) | ||
| smape = None | ||
| valid_smape = smape_denominator > 0 | ||
| if np.any(valid_smape): | ||
| smape = float( | ||
| 200.0 | ||
| * np.mean(absolute_errors[valid_smape] / smape_denominator[valid_smape]) | ||
| ) | ||
| else: | ||
| reasons["smape"] = "sMAPE is undefined when actual and forecast are both zero." | ||
|
|
||
| return ForecastMetrics( | ||
| rmse=float(np.sqrt(np.mean(errors**2))), | ||
| mae=float(np.mean(absolute_errors)), | ||
| mape=mape, | ||
| wape=wape, | ||
| mase=mase, | ||
| smape=smape, | ||
| rmsse=rmsse, | ||
| n_evaluated=int(y_true.size), | ||
| n_missing=n_missing, | ||
| unavailable_reasons=reasons, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cognitive complexity CI check fails on calculate_forecast_metrics.
SonarCloud reports cognitive complexity 23 vs. the allowed 15 for this function (CI failure). Consider extracting per-metric helpers (e.g. _mape, _wape, _mase_rmsse, _smape) to bring this under the threshold while keeping behavior identical.
def _mase_rmsse(errors, absolute_errors, training, mase_period):
"""Return (mase, rmsse, reasons) or (None, None, reasons)."""
...🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 55-55: Refactor this function to reduce its Cognitive Complexity from 23 to the 15 allowed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/forecasting/metrics.py` around lines 55 - 154, Reduce
the cognitive complexity of calculate_forecast_metrics by extracting the
metric-specific branches into focused helpers, such as _mape, _wape,
_mase_rmsse, and _smape. Keep calculate_forecast_metrics responsible for input
filtering, orchestration, and assembling ForecastMetrics, while preserving all
existing values, unavailable_reasons, and edge-case behavior.
Source: Linters/SAST tools
| def _variance_by_horizon( | ||
| fold_residuals: Sequence[Sequence[float]], | ||
| ) -> dict[int, float]: | ||
| """Compute error variance keyed by horizon step across folds.""" | ||
| by_horizon: dict[int, list[float]] = {} | ||
| for residuals in fold_residuals: | ||
| for h, value in enumerate(residuals): | ||
| by_horizon.setdefault(h, []).append(float(value)) | ||
| return { | ||
| h: float(np.var(values, ddof=1)) if len(values) > 1 else 0.0 | ||
| for h, values in sorted(by_horizon.items()) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
_variance_by_horizon is 0-indexed while sibling *_by_horizon maps are 1-indexed.
by_horizon keys come straight from enumerate(residuals) (0, 1, 2, …), but interval_coverage_by_horizon/interval_width_by_horizon/winkler_score_by_horizon (358-389) use step = horizon + 1 (1, 2, 3, …). Both sets of maps live on the same ResidualDiagnosticsResult; any consumer that joins them by horizon key will be off by one.
🩹 Proposed fix
for residuals in fold_residuals:
for h, value in enumerate(residuals):
- by_horizon.setdefault(h, []).append(float(value))
+ by_horizon.setdefault(h + 1, []).append(float(value))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _variance_by_horizon( | |
| fold_residuals: Sequence[Sequence[float]], | |
| ) -> dict[int, float]: | |
| """Compute error variance keyed by horizon step across folds.""" | |
| by_horizon: dict[int, list[float]] = {} | |
| for residuals in fold_residuals: | |
| for h, value in enumerate(residuals): | |
| by_horizon.setdefault(h, []).append(float(value)) | |
| return { | |
| h: float(np.var(values, ddof=1)) if len(values) > 1 else 0.0 | |
| for h, values in sorted(by_horizon.items()) | |
| } | |
| def _variance_by_horizon( | |
| fold_residuals: Sequence[Sequence[float]], | |
| ) -> dict[int, float]: | |
| """Compute error variance keyed by horizon step across folds.""" | |
| by_horizon: dict[int, list[float]] = {} | |
| for residuals in fold_residuals: | |
| for h, value in enumerate(residuals): | |
| by_horizon.setdefault(h + 1, []).append(float(value)) | |
| return { | |
| h: float(np.var(values, ddof=1)) if len(values) > 1 else 0.0 | |
| for h, values in sorted(by_horizon.items()) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/forecasting/residual_diagnostics.py` around lines 94
- 105, Update _variance_by_horizon to use 1-based horizon keys, matching the
sibling interval_coverage_by_horizon, interval_width_by_horizon, and
winkler_score_by_horizon maps. Adjust the residual enumeration so the first
value is stored under horizon 1 while preserving the existing variance
calculation and sorted output.
| @staticmethod | ||
| def _forecast_pattern(forecast: ForecastResult) -> str: | ||
| """Classify the plotted path without confusing endpoints with trend.""" | ||
| values = np.asarray(forecast.forecast, dtype=float) | ||
| if values.size < 2: | ||
| return "Flat" | ||
| changes = np.diff(values) | ||
| tolerance = max(float(np.nanmax(np.abs(values))) * 1e-9, 1e-12) | ||
| if np.all(changes >= -tolerance): | ||
| return "Upward" | ||
| if np.all(changes <= tolerance): | ||
| return "Downward" | ||
| return "Seasonal / variable" | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Flat/constant forecasts are misclassified as "Upward" here too.
Same issue as report/dashboard.py's forecast_pattern_status: when all np.diff(values) are 0, np.all(changes >= -tolerance) is True, so a genuinely flat/constant forecast (e.g. "Constant"/"Naive" fallback) is reported as "Upward" instead of "Flat", contradicting endpoint_direction computed a few lines later in _build_forecast_metrics (which correctly resolves to "Flat" for pct_change == 0).
🐛 Proposed fix
`@staticmethod`
def _forecast_pattern(forecast: ForecastResult) -> str:
"""Classify the plotted path without confusing endpoints with trend."""
values = np.asarray(forecast.forecast, dtype=float)
if values.size < 2:
return "Flat"
changes = np.diff(values)
tolerance = max(float(np.nanmax(np.abs(values))) * 1e-9, 1e-12)
+ if np.all(np.abs(changes) <= tolerance):
+ return "Flat"
if np.all(changes >= -tolerance):
return "Upward"
if np.all(changes <= tolerance):
return "Downward"
return "Seasonal / variable"See consolidated comment covering both this and report/dashboard.py's equivalent function.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @staticmethod | |
| def _forecast_pattern(forecast: ForecastResult) -> str: | |
| """Classify the plotted path without confusing endpoints with trend.""" | |
| values = np.asarray(forecast.forecast, dtype=float) | |
| if values.size < 2: | |
| return "Flat" | |
| changes = np.diff(values) | |
| tolerance = max(float(np.nanmax(np.abs(values))) * 1e-9, 1e-12) | |
| if np.all(changes >= -tolerance): | |
| return "Upward" | |
| if np.all(changes <= tolerance): | |
| return "Downward" | |
| return "Seasonal / variable" | |
| `@staticmethod` | |
| def _forecast_pattern(forecast: ForecastResult) -> str: | |
| """Classify the plotted path without confusing endpoints with trend.""" | |
| values = np.asarray(forecast.forecast, dtype=float) | |
| if values.size < 2: | |
| return "Flat" | |
| changes = np.diff(values) | |
| tolerance = max(float(np.nanmax(np.abs(values))) * 1e-9, 1e-12) | |
| if np.all(np.abs(changes) <= tolerance): | |
| return "Flat" | |
| if np.all(changes >= -tolerance): | |
| return "Upward" | |
| if np.all(changes <= tolerance): | |
| return "Downward" | |
| return "Seasonal / variable" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/report/builder.py` around lines 603 - 616, Update
_forecast_pattern to detect genuinely constant forecasts before the
nondecreasing/nonincreasing checks and return "Flat" when all changes are within
the existing tolerance, keeping upward and downward classification for non-flat
trends and preserving the existing short-input behavior.
| rmse=round(forecast.rmse, 4) if forecast.rmse is not None else None, | ||
| mae=round(forecast.mae, 4) if forecast.mae is not None else None, | ||
| mape=round(forecast.mape, 2) if forecast.mape is not None else None, | ||
| wape=round(forecast.wape, 2) if forecast.wape is not None else None, | ||
| mase=round(forecast.mase, 4) if forecast.mase is not None else None, | ||
| interval_label=interval_label, | ||
| prediction_intervals=intervals, | ||
| selection_metrics=forecast.selection_metrics, | ||
| final_test_metrics=forecast.final_test_metrics, | ||
| final_test_assessment=final_test_assessment, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep run --pattern 'def _build_appendix($$$) { $$$ }' --lang python data_forecaster/backend/report/builder.py
rg -n 'NOT_ESTIMABLE|FAILED' --type=py data_forecaster/backend/agents/forecasting_agent.py data_forecaster/backend/services/pipeline_service.pyRepository: bmfmancini/data_forecasting_agent
Length of output: 717
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
paths = [
Path("data_forecaster/backend/report/builder.py"),
Path("data_forecaster/backend/schemas.py"),
Path("data_forecaster/backend/agents/forecasting_agent.py"),
Path("data_forecaster/backend/services/pipeline_service.py"),
]
for path in paths:
print(f"\n=== {path} ===")
if path.exists():
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(key in line for key in [
"def _build_appendix",
"def _build_forecast_metrics",
"def build(",
"rmse=",
"mae=",
"mape=",
"wape=",
"mase=",
"ForecastResult",
"NOT_ESTIMABLE",
"FAILED",
]):
start = max(1, i - 8)
end = min(len(lines), i + 20)
print(f"\n--- around line {i} ---")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
else:
print("missing")
PYRepository: bmfmancini/data_forecasting_agent
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
targets = [
("data_forecaster/backend/agents/forecasting_agent.py", [
"ForecastResult(",
"status=ForecastFitStatus.FAILED",
"status=ForecastFitStatus.NOT_ESTIMABLE",
"rmse=",
"mae=",
"mape=",
"wape=",
"mase=",
"fallback",
]),
("data_forecaster/backend/services/pipeline_service.py", [
"ForecastResult(",
"NOT_ESTIMABLE",
"FAILED",
"forecast_result",
"all_metrics",
]),
]
for rel, needles in targets:
path = Path(rel)
print(f"\n=== {rel} ===")
lines = path.read_text().splitlines()
hits = []
for i, line in enumerate(lines, 1):
if any(n in line for n in needles):
hits.append(i)
for i in hits[:40]:
start = max(1, i - 8)
end = min(len(lines), i + 18)
print(f"\n--- around line {i} ---")
for j in range(start, end + 1):
print(f"{j:4d}: {lines[j-1]}")
PYRepository: bmfmancini/data_forecasting_agent
Length of output: 50389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A12 -B8 \
'class ForecastAdapterResult|class ForecastMetrics|class ForecastEvaluation|class Backtest|metrics:|pooled_metrics|is_rankable|NOT_ESTIMABLE|ForecastFitStatus' \
data_forecaster/backend -g '*.py'Repository: bmfmancini/data_forecasting_agent
Length of output: 50390
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = {
"data_forecaster/backend/agents/model_selection_agent.py": [
817, 860
],
"data_forecaster/backend/agents/forecasting_agent.py": [
780, 900
],
"data_forecaster/backend/report/models.py": [
160, 210
],
}
for rel, (start, end) in files.items():
path = Path(rel)
print(f"\n=== {rel} {start}-{end} ===")
lines = path.read_text().splitlines()
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: bmfmancini/data_forecasting_agent
Length of output: 10008
Guard nullable forecast metrics in _build_appendix _build_appendix still calls round(forecast.rmse, 4), round(forecast.mae, 4), and round(forecast.mape, 2) unconditionally. ForecastResult.rmse/mae/mape are nullable, and the forecast pipeline can pass through None on failed/not-estimable paths, which will raise TypeError while building the report. Mirror the None guards used in _build_forecast_metrics here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/report/builder.py` around lines 719 - 729, Update
`_build_appendix` to guard nullable `forecast.rmse`, `forecast.mae`, and
`forecast.mape` values before rounding, matching the existing conditional
pattern in `_build_forecast_metrics`. Preserve the current rounding precision
and return `None` when each metric is unavailable.
| def forecast_pattern_status(values: list[float]) -> tuple[str, str]: | ||
| """Classify monotonic direction separately from a variable seasonal path.""" | ||
| if len(values) < 2: | ||
| return FORECAST_DIRECTIONS["flat"], "neutral" | ||
| changes = [current - previous for previous, current in zip(values, values[1:])] | ||
| tolerance = max(max(abs(value) for value in values) * 1e-9, 1e-12) | ||
| if all(change >= -tolerance for change in changes): | ||
| return FORECAST_DIRECTIONS["upward"], "positive" | ||
| if all(change <= tolerance for change in changes): | ||
| return FORECAST_DIRECTIONS["downward"], "negative" | ||
| return "Seasonal / Variable", "info" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Flat/constant forecasts are misclassified as "Upward".
When every value is equal, all changes are 0, which satisfies all(change >= -tolerance ...) before the downward check is ever reached, so the pattern is reported as "Upward" instead of "Flat". This contradicts the sibling endpoint_direction field (correctly "Flat" when pct_change == 0), producing an internally inconsistent dashboard for common baseline/fallback forecasts (e.g. "Constant"/"Naive").
🐛 Proposed fix
def forecast_pattern_status(values: list[float]) -> tuple[str, str]:
"""Classify monotonic direction separately from a variable seasonal path."""
if len(values) < 2:
return FORECAST_DIRECTIONS["flat"], "neutral"
changes = [current - previous for previous, current in zip(values, values[1:])]
tolerance = max(max(abs(value) for value in values) * 1e-9, 1e-12)
+ if all(abs(change) <= tolerance for change in changes):
+ return FORECAST_DIRECTIONS["flat"], "neutral"
if all(change >= -tolerance for change in changes):
return FORECAST_DIRECTIONS["upward"], "positive"
if all(change <= tolerance for change in changes):
return FORECAST_DIRECTIONS["downward"], "negative"
return "Seasonal / Variable", "info"Note: the identical bug (and a capitalization drift — "Seasonal / Variable" here vs "Seasonal / variable" in builder.py's _forecast_pattern) exists in data_forecaster/backend/report/builder.py — see consolidated comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def forecast_pattern_status(values: list[float]) -> tuple[str, str]: | |
| """Classify monotonic direction separately from a variable seasonal path.""" | |
| if len(values) < 2: | |
| return FORECAST_DIRECTIONS["flat"], "neutral" | |
| changes = [current - previous for previous, current in zip(values, values[1:])] | |
| tolerance = max(max(abs(value) for value in values) * 1e-9, 1e-12) | |
| if all(change >= -tolerance for change in changes): | |
| return FORECAST_DIRECTIONS["upward"], "positive" | |
| if all(change <= tolerance for change in changes): | |
| return FORECAST_DIRECTIONS["downward"], "negative" | |
| return "Seasonal / Variable", "info" | |
| def forecast_pattern_status(values: list[float]) -> tuple[str, str]: | |
| """Classify monotonic direction separately from a variable seasonal path.""" | |
| if len(values) < 2: | |
| return FORECAST_DIRECTIONS["flat"], "neutral" | |
| changes = [current - previous for previous, current in zip(values, values[1:])] | |
| tolerance = max(max(abs(value) for value in values) * 1e-9, 1e-12) | |
| if all(abs(change) <= tolerance for change in changes): | |
| return FORECAST_DIRECTIONS["flat"], "neutral" | |
| if all(change >= -tolerance for change in changes): | |
| return FORECAST_DIRECTIONS["upward"], "positive" | |
| if all(change <= tolerance for change in changes): | |
| return FORECAST_DIRECTIONS["downward"], "negative" | |
| return "Seasonal / Variable", "info" |
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 125-125: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 125-125: Prefer itertools.pairwise() over zip() when iterating over successive pairs
Replace zip() with itertools.pairwise()
(RUF007)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/report/dashboard.py` around lines 121 - 131, Update
forecast_pattern_status to detect when all values are equal, or all changes are
within the existing tolerance, and return the flat direction with the
appropriate neutral status before the upward check. Apply the same flat-case
correction in builder.py’s _forecast_pattern and align its “Seasonal / Variable”
capitalization with forecast_pattern_status.
|
|
||
| def _unexpected_model_references(text: str, expected_model: str) -> list[str]: | ||
| """Reject forecast prose that names a model other than the fitted model.""" | ||
| normalized = re.sub(r"[‐‑‒–—−]", "-", text).lower() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Extract the duplicated dash-normalization regex into a shared constant.
SonarCloud flags r"[‐‑‒–—−]" as duplicated 4× across these functions (a CI-reported failure). Centralize it once, e.g. a module-level compiled pattern, to avoid drift and satisfy the analysis check; the Ruff RUF001 ambiguous-character warnings on the same lines are expected since these dash glyphs are intentionally being normalized from LLM output — consider a # noqa: RUF001 on the single constant definition once consolidated.
♻️ Suggested fix
+_DASH_VARIANTS_RE = re.compile(r"[‐‑‒–—−]") # noqa: RUF001
+
+
def _unexpected_model_references(text: str, expected_model: str) -> list[str]:
"""Reject forecast prose that names a model other than the fitted model."""
- normalized = re.sub(r"[‐‑‒–—−]", "-", text).lower()
+ normalized = _DASH_VARIANTS_RE.sub("-", text).lower()
...
def _contradictory_model_selection(text: str, expected_model: str) -> list[str]:
"""Reject prose that attributes selection to a different named model."""
- normalized = re.sub(r"[‐‑‒–—−]", "-", text)
+ normalized = _DASH_VARIANTS_RE.sub("-", text)
...
def _contradictory_forecast_pattern(text: str, pattern: str) -> list[str]:
...
- normalized = re.sub(r"[‐‑‒–—−]", "-", text).lower()
+ normalized = _DASH_VARIANTS_RE.sub("-", text).lower()
...
def _unsupported_recommendation_claims(text: str, section_data: dict[str, Any]) -> list[str]:
"""Reject recommendation prose that reverses deterministic safeguards."""
- normalized = re.sub(r"[‐‑‒–—−]", "-", text).lower()
+ normalized = _DASH_VARIANTS_RE.sub("-", text).lower()Also applies to: 286-286, 306-306, 397-397
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[failure] 264-264: Define a constant instead of duplicating this literal r"[‐‑‒–—−]" 4 times.
🪛 Ruff (0.15.21)
[warning] 264-264: String contains ambiguous ‐ (HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF001)
[warning] 264-264: String contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF001)
[warning] 264-264: String contains ambiguous ‒ (FIGURE DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
[warning] 264-264: String contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF001)
[warning] 264-264: String contains ambiguous − (MINUS SIGN). Did you mean - (HYPHEN-MINUS)?
(RUF001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/report/narrative.py` at line 264, Extract the
repeated dash-normalization regex used by the affected functions into one
module-level compiled pattern in narrative.py. Replace all four inline re.sub
patterns with that shared constant, preserving the existing lowercase
normalization behavior, and add the narrowly scoped RUF001 suppression on the
constant definition if required.
Source: Linters/SAST tools
| def terminal_holdout_validation( | ||
| series: pd.Series, | ||
| forecast_horizon: int, | ||
| forecast_fn: ForecastFunction, | ||
| ) -> dict[str, float]: |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return type annotation doesn't match model_dump() output.
ForecastMetrics.model_dump() includes n_evaluated/n_missing (ints) and unavailable_reasons (dict[str, str]), not just floats, so -> dict[str, float] misleads callers/type-checkers.
As per coding guidelines, "typed public APIs" is required for Python modules.
🩹 Proposed fix
def terminal_holdout_validation(
series: pd.Series,
forecast_horizon: int,
forecast_fn: ForecastFunction,
-) -> dict[str, float]:
+) -> dict[str, object]:Also applies to: 56-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@data_forecaster/backend/utils/validation.py` around lines 20 - 24, Update the
return annotation of terminal_holdout_validation to match the full
ForecastMetrics.model_dump() result, including integer n_evaluated/n_missing
fields and the unavailable_reasons mapping. Reuse the existing
ForecastMetrics-derived or equivalent typed mapping rather than dict[str,
float], and apply the same correction to the additional affected return
annotation.
Source: Coding guidelines
address code rabbit concerns
|
1 participant
Uh oh!
There was an error while loading. Please reload this page.