fix: address PR review issues for event comparison and backtests
Critical fixes: - Add validate_and_calculate_units() helper with proper error handling - Handle division by zero for entry_spot in refresh_preview() and render_report() - Add server-side validation for initial_value > 0 - Add try/except for derive_entry_spot() to handle fixture source limitations Important improvements: - Add dynamic default dates with get_default_backtest_dates() - Add validate_date_range_for_symbol() for symbol-specific date bounds - Add SYMBOL_MIN_DATES validation for backtests - Update date_range_hint based on selected symbol Tests: - Add test_page_validation.py with 21 tests for: - validate_and_calculate_units edge cases - validate_date_range_for_symbol bounds checking - get_default_backtest_dates dynamic generation - SYMBOL_MIN_DATES constant verification
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import logging
|
||||
from datetime import date, datetime
|
||||
from datetime import date, datetime, timedelta
|
||||
from typing import Any
|
||||
|
||||
from fastapi.responses import RedirectResponse
|
||||
@@ -48,9 +48,36 @@ SYMBOL_MIN_DATES = {
|
||||
"XAU": date(1970, 1, 1), # XAU index historical data
|
||||
}
|
||||
|
||||
# Reasonable default date range (2 years of data)
|
||||
DEFAULT_BACKTEST_START = "2022-01-03" # First trading day of 2022
|
||||
DEFAULT_BACKTEST_END = "2023-12-29" # Last trading day of 2023
|
||||
def get_default_backtest_dates() -> tuple[date, date]:
|
||||
"""Get default backtest date range (last 2 years excluding current week)."""
|
||||
today = date.today()
|
||||
# Find the most recent Friday that's at least a week old
|
||||
days_since_friday = (today.weekday() - 4) % 7
|
||||
if days_since_friday == 0 and today.weekday() != 4:
|
||||
# Not Friday yet, go back to previous Friday
|
||||
days_since_friday = 7
|
||||
end = today - timedelta(days=days_since_friday)
|
||||
start = end - timedelta(days=730) # ~2 years
|
||||
return start, end
|
||||
|
||||
|
||||
DEFAULT_BACKTEST_START = get_default_backtest_dates()[0].isoformat()
|
||||
DEFAULT_BACKTEST_END = get_default_backtest_dates()[1].isoformat()
|
||||
|
||||
|
||||
def validate_date_range_for_symbol(start_date: date, end_date: date, symbol: str) -> str | None:
|
||||
"""Validate date range is within available data for symbol.
|
||||
|
||||
Returns error message if invalid, None if valid.
|
||||
"""
|
||||
min_date = SYMBOL_MIN_DATES.get(symbol)
|
||||
if min_date and start_date < min_date:
|
||||
return f"Start date must be on or after {min_date.strftime('%Y-%m-%d')} for {symbol} (data availability)."
|
||||
if end_date > date.today():
|
||||
return "End date cannot be in the future."
|
||||
if start_date > end_date:
|
||||
return "Start date must be before or equal to end date."
|
||||
return None
|
||||
|
||||
|
||||
def _chart_options(result: BacktestPageRunResult) -> dict:
|
||||
@@ -199,11 +226,17 @@ def _render_backtests_page(workspace_id: str | None = None) -> None:
|
||||
default_start_price = 0.0
|
||||
|
||||
# Derive entry spot from default date range
|
||||
default_entry_spot = service.derive_entry_spot(
|
||||
"GLD",
|
||||
date.fromisoformat(DEFAULT_BACKTEST_START),
|
||||
date.fromisoformat(DEFAULT_BACKTEST_END),
|
||||
)
|
||||
# Fall back to a reasonable default if fixture source doesn't support the date range
|
||||
try:
|
||||
default_entry_spot = service.derive_entry_spot(
|
||||
"GLD",
|
||||
date.fromisoformat(DEFAULT_BACKTEST_START),
|
||||
date.fromisoformat(DEFAULT_BACKTEST_END),
|
||||
)
|
||||
except ValueError:
|
||||
# Fixture source may not support the default date range
|
||||
# Fall back to a reasonable GLD price
|
||||
default_entry_spot = 185.0
|
||||
default_units = (
|
||||
asset_quantity_from_workspace_config(config, entry_spot=default_entry_spot, symbol="GLD")
|
||||
if config is not None and default_entry_spot > 0
|
||||
@@ -311,6 +344,9 @@ def _render_backtests_page(workspace_id: str | None = None) -> None:
|
||||
f"GLD data available from {SYMBOL_MIN_DATES['GLD'].strftime('%Y-%m-%d')} (ETF launch)"
|
||||
).classes("text-xs text-slate-500 dark:text-slate-400")
|
||||
|
||||
# Note: date_range_hint will be updated when symbol changes via on_value_change
|
||||
# The get_symbol_from_dataset function is defined later and referenced in the callback
|
||||
|
||||
start_price_input = (
|
||||
ui.number("Start price (0 = auto-derive)", value=default_start_price, min=0, step=0.01)
|
||||
.classes("w-full")
|
||||
@@ -380,6 +416,17 @@ def _render_backtests_page(workspace_id: str | None = None) -> None:
|
||||
return "GC"
|
||||
return "GLD" # Default for XNAS.BASIC
|
||||
|
||||
def update_date_range_hint() -> None:
|
||||
"""Update the date range hint based on selected symbol."""
|
||||
symbol = get_symbol_from_dataset()
|
||||
min_date = SYMBOL_MIN_DATES.get(symbol)
|
||||
if min_date:
|
||||
date_range_hint.set_text(
|
||||
f"{symbol} data available from {min_date.strftime('%Y-%m-%d')}"
|
||||
)
|
||||
else:
|
||||
date_range_hint.set_text(f"{symbol} data availability unknown")
|
||||
|
||||
def update_cost_estimate() -> None:
|
||||
"""Update cost estimate display based on current settings."""
|
||||
current_data_source = str(data_source_select.value)
|
||||
@@ -607,9 +654,17 @@ def _render_backtests_page(workspace_id: str | None = None) -> None:
|
||||
).classes("w-full")
|
||||
|
||||
def validate_current_scenario(*, entry_spot: float | None = None) -> str | None:
|
||||
# Validate date range against symbol data availability
|
||||
start_date = parse_iso_date(start_input.value, "Start date")
|
||||
end_date = parse_iso_date(end_input.value, "End date")
|
||||
symbol = get_symbol_from_dataset()
|
||||
date_range_error = validate_date_range_for_symbol(start_date, end_date, symbol)
|
||||
if date_range_error:
|
||||
return date_range_error
|
||||
|
||||
try:
|
||||
service.validate_preview_inputs(
|
||||
symbol=get_symbol_from_dataset(),
|
||||
symbol=symbol,
|
||||
start_date=parse_iso_date(start_input.value, "Start date"),
|
||||
end_date=parse_iso_date(end_input.value, "End date"),
|
||||
template_slug=str(template_select.value or ""),
|
||||
@@ -716,13 +771,23 @@ def _render_backtests_page(workspace_id: str | None = None) -> None:
|
||||
def run_backtest() -> None:
|
||||
validation_label.set_text("")
|
||||
try:
|
||||
# Validate date range for symbol
|
||||
start_date = parse_iso_date(start_input.value, "Start date")
|
||||
end_date = parse_iso_date(end_input.value, "End date")
|
||||
symbol = get_symbol_from_dataset()
|
||||
date_range_error = validate_date_range_for_symbol(start_date, end_date, symbol)
|
||||
if date_range_error:
|
||||
validation_label.set_text(date_range_error)
|
||||
render_result_state("Scenario validation failed", date_range_error, tone="warning")
|
||||
return
|
||||
|
||||
# Save settings before running
|
||||
save_backtest_settings()
|
||||
|
||||
result = service.run_read_only_scenario(
|
||||
symbol=get_symbol_from_dataset(),
|
||||
start_date=parse_iso_date(start_input.value, "Start date"),
|
||||
end_date=parse_iso_date(end_input.value, "End date"),
|
||||
symbol=symbol,
|
||||
start_date=start_date,
|
||||
end_date=end_date,
|
||||
template_slug=str(template_select.value or ""),
|
||||
underlying_units=float(units_input.value or 0.0),
|
||||
loan_amount=float(loan_input.value or 0.0),
|
||||
@@ -765,7 +830,7 @@ def _render_backtests_page(workspace_id: str | None = None) -> None:
|
||||
data_source_select.on_value_change(lambda e: on_form_change())
|
||||
dataset_select.on_value_change(lambda e: on_form_change())
|
||||
schema_select.on_value_change(lambda e: on_form_change())
|
||||
symbol_select.on_value_change(lambda e: on_form_change())
|
||||
symbol_select.on_value_change(lambda e: (update_date_range_hint(), on_form_change()))
|
||||
start_input.on_value_change(lambda e: refresh_workspace_seeded_units())
|
||||
end_input.on_value_change(lambda e: refresh_workspace_seeded_units())
|
||||
start_price_input.on_value_change(lambda e: on_form_change())
|
||||
|
||||
@@ -13,6 +13,18 @@ from app.services.event_comparison_ui import EventComparisonPageService
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def validate_and_calculate_units(initial_value: float, entry_spot: float) -> tuple[float, str | None]:
|
||||
"""Validate inputs and calculate underlying units.
|
||||
|
||||
Returns (units, error_message). If error_message is not None, units is 0.0.
|
||||
"""
|
||||
if initial_value <= 0:
|
||||
return 0.0, "Initial portfolio value must be positive."
|
||||
if entry_spot <= 0:
|
||||
return 0.0, "Cannot calculate units: entry spot is invalid. Please select a valid preset."
|
||||
return initial_value / entry_spot, None
|
||||
|
||||
|
||||
def _chart_options(dates: tuple[str, ...], series: tuple[dict[str, object], ...]) -> dict:
|
||||
return {
|
||||
"tooltip": {"trigger": "axis"},
|
||||
@@ -143,10 +155,13 @@ def _render_event_comparison_page(workspace_id: str | None = None) -> None:
|
||||
selected_summary.clear()
|
||||
with selected_summary:
|
||||
ui.label("Scenario Summary").classes("text-lg font-semibold text-slate-900 dark:text-slate-100")
|
||||
# Calculate underlying units from initial value and entry spot
|
||||
computed_units = 0.0
|
||||
if entry_spot is not None and entry_spot > 0:
|
||||
computed_units = float(initial_value_input.value or 0.0) / entry_spot
|
||||
# Calculate underlying units with validation
|
||||
initial_value = float(initial_value_input.value or 0.0)
|
||||
computed_units, units_error = (
|
||||
validate_and_calculate_units(initial_value, entry_spot)
|
||||
if entry_spot is not None
|
||||
else (0.0, "Entry spot unavailable.")
|
||||
)
|
||||
with ui.grid(columns=1).classes("w-full gap-4 sm:grid-cols-2 lg:grid-cols-1 xl:grid-cols-2"):
|
||||
cards = [
|
||||
(
|
||||
@@ -165,8 +180,11 @@ def _render_event_comparison_page(workspace_id: str | None = None) -> None:
|
||||
):
|
||||
ui.label(label).classes("text-sm text-slate-500 dark:text-slate-400")
|
||||
ui.label(value).classes("text-xl font-bold text-slate-900 dark:text-slate-100")
|
||||
if entry_spot_error:
|
||||
ui.label(entry_spot_error).classes("text-sm text-amber-700 dark:text-amber-300")
|
||||
# Show validation errors (units_error takes priority, then entry_spot_error)
|
||||
display_error = units_error or entry_spot_error
|
||||
if display_error:
|
||||
tone_class = "text-rose-600 dark:text-rose-300" if "must be positive" in display_error else "text-amber-700 dark:text-amber-300"
|
||||
ui.label(display_error).classes(f"text-sm {tone_class}")
|
||||
|
||||
def render_result_state(title: str, message: str, *, tone: str = "info") -> None:
|
||||
tone_classes = {
|
||||
@@ -218,8 +236,13 @@ def _render_event_comparison_page(workspace_id: str | None = None) -> None:
|
||||
preset_slug=str(option["slug"]),
|
||||
template_slugs=template_slugs,
|
||||
)
|
||||
# Calculate underlying units from initial value and entry spot
|
||||
preview_units = preview_initial_value / preview_entry_spot if preview_entry_spot > 0 else 0.0
|
||||
# Validate and calculate underlying units
|
||||
preview_units, units_error = validate_and_calculate_units(preview_initial_value, preview_entry_spot)
|
||||
if units_error:
|
||||
metadata_label.set_text(f"Preset: {option['label']} — {option['description']}")
|
||||
scenario_label.set_text(units_error)
|
||||
render_selected_summary(entry_spot=preview_entry_spot, entry_spot_error=units_error)
|
||||
return units_error
|
||||
|
||||
if workspace_id and config is not None and reseed_units:
|
||||
# Recalculate from workspace config
|
||||
@@ -282,7 +305,7 @@ def _render_event_comparison_page(workspace_id: str | None = None) -> None:
|
||||
result_panel.clear()
|
||||
template_slugs = selected_template_slugs()
|
||||
try:
|
||||
# Get initial portfolio value and calculate underlying units
|
||||
# Get initial portfolio value and calculate underlying units with validation
|
||||
initial_value = float(initial_value_input.value or 0.0)
|
||||
# Get entry spot from preview
|
||||
option = preset_lookup.get(str(preset_select.value or ""))
|
||||
@@ -293,7 +316,12 @@ def _render_event_comparison_page(workspace_id: str | None = None) -> None:
|
||||
preset_slug=str(option["slug"]),
|
||||
template_slugs=template_slugs,
|
||||
)
|
||||
underlying_units = initial_value / entry_spot if entry_spot > 0 else 0.0
|
||||
# Validate and calculate underlying units
|
||||
underlying_units, units_error = validate_and_calculate_units(initial_value, entry_spot)
|
||||
if units_error:
|
||||
validation_label.set_text(units_error)
|
||||
render_result_state("Input validation failed", units_error, tone="warning")
|
||||
return
|
||||
|
||||
report = service.run_read_only_comparison(
|
||||
preset_slug=str(preset_select.value or ""),
|
||||
|
||||
Reference in New Issue
Block a user