Merge pull request #512 from coding-kitties/fix/issue-511

fix(backtesting): aggregate summary.json across all walk-forward runs (#511)

Author: MDUYN

investing_algorithm_framework/domain/backtesting/backtest.py

@@ -418,6 +418,23 @@ def save(
                 os.makedirs(destination_run_path, exist_ok=True)
                 br.save(destination_run_path)
 
+        # Always rebuild the summary from the current set of backtest runs
+        # before writing it to disk. This guarantees that summary.json is
+        # self-consistent with the per-run metrics.json files (e.g.
+        # number_of_windows == number of runs, total_net_gain == sum of
+        # per-run total_net_gain, etc.) regardless of how the in-memory
+        # Backtest was constructed (single backtest, walk-forward,
+        # merge(), …). See issue #511.
+        if self.backtest_runs:
+            per_run_metrics = [
+                br.backtest_metrics for br in self.backtest_runs
+                if br.backtest_metrics is not None
+            ]
+            if per_run_metrics:
+                self.backtest_summary = generate_backtest_summary_metrics(
+                    per_run_metrics
+                )
+
         # Save combined backtest metrics if available
         if self.backtest_summary:
             summary_file = os.path.join(
@@ -517,15 +534,22 @@ def merge(self, other: 'Backtest') -> 'Backtest':
             Backtest: The merged Backtest instance.
         """
 
-        merged = Backtest()
+        merged = Backtest(algorithm_id=self.algorithm_id)
         merged.backtest_runs = self.backtest_runs + other.backtest_runs
 
-        summary = BacktestSummaryMetrics()
-
-        for bt_run in merged.get_all_backtest_metrics():
-            summary.add(bt_run)
+        # Rebuild the summary from the full set of merged backtest runs.
+        # `BacktestSummaryMetrics` is a plain dataclass and does not expose
+        # an `add()` method, so the previous incremental approach raised
+        # AttributeError and silently produced a stale / single-window
+        # summary. See issue #511.
+        merged_metrics = merged.get_all_backtest_metrics()
+        if merged_metrics:
+            merged.backtest_summary = generate_backtest_summary_metrics(
+                merged_metrics
+            )
+        else:
+            merged.backtest_summary = BacktestSummaryMetrics()
 
-        merged.backtest_summary = summary
         merged.backtest_permutation_tests = \
             self.backtest_permutation_tests + other.backtest_permutation_tests
 

investing_algorithm_framework/domain/backtesting/backtest_metrics.py

@@ -20,6 +20,21 @@ class BacktestMetrics:
     total return, annualized return, volatility, Sharpe ratio,
     and maximum drawdown.
 
+    .. note:: Field semantics & known duplicates (issue #511)
+
+        - ``total_loss`` is the **gross loss magnitude** (a non-negative
+          number equal to ``sum(abs(net_gain))`` over losing trades).
+          ``total_loss_percentage`` is ``total_loss /
+          initial_unallocated`` (decimal, non-negative). These fields
+          no longer mirror ``total_net_gain`` / ``total_net_gain_pct``;
+          see B1 in issue #511.
+
+        - ``total_growth`` / ``total_growth_percentage`` are
+          numerically equivalent to ``total_net_gain`` /
+          ``total_net_gain_percentage`` for the standard
+          mark-to-market portfolio used in backtests. They are kept as
+          legacy aliases. See B3 in issue #511.
+
     Attributes:
         backtest_date_range_name (str): The name of the date range
             used for the backtest.

investing_algorithm_framework/domain/backtesting/backtest_summary_metrics.py

@@ -13,6 +13,30 @@ class BacktestSummaryMetrics:
     Represents the summarized results of a backtest,
     focusing on key headline performance and risk metrics.
 
+    .. note:: Field semantics & known duplicates (issue #511)
+
+        - ``total_loss`` / ``total_loss_percentage`` are gross-loss
+          based: ``total_loss`` is ``sum(per-run gross_loss)`` (a
+          non-negative magnitude in account currency) and
+          ``total_loss_percentage`` is ``total_loss /
+          sum(initial_unallocated)`` (decimal). They no longer mix
+          with net-return semantics. See B1/B2 in issue #511.
+
+        - ``total_growth`` / ``total_growth_percentage`` are
+          numerically equivalent to ``total_net_gain`` /
+          ``total_net_gain_percentage`` for closed-position backtests
+          because both are derived from the same start/end portfolio
+          values. They are kept for backwards compatibility but should
+          be considered legacy aliases. See B3 in issue #511.
+
+        - ``average_net_gain``, ``average_loss``, ``average_growth``
+          (and their ``*_percentage`` counterparts) are time-weighted
+          means **across windows**. For a single-window backtest they
+          collapse to the corresponding ``total_*`` value by
+          definition (weighted mean of one element equals that
+          element). See B4 in issue #511. For per-trade averages use
+          ``average_trade_*`` instead.
+
     Attributes:
         total_net_gain (float): Total net gain from the backtest.
         total_net_gain_percentage (float): Total net gain percentage

investing_algorithm_framework/domain/backtesting/combine_backtests.py

@@ -39,28 +39,32 @@ def _compound_percentage_returns(percentages):
     Compound percentage returns across multiple periods.
 
     For example, if period 1 has 10% return and period 2 has 5% return,
-    the compounded return is: (1 + 0.10) * (1 + 0.05) - 1 = 15.5%
-    NOT simply 10% + 5% = 15%
+    the compounded return is: (1 + 0.10) * (1 + 0.05) - 1 = 0.155 = 15.5%
+    NOT simply 0.10 + 0.05 = 0.15.
+
+    The framework consistently represents percentages as **decimals**
+    (e.g. ``0.10`` for 10%), so this helper expects decimal inputs and
+    returns a decimal. See issue #511 (B5) — earlier versions assumed
+    whole-number percentages, which silently produced results off by a
+    factor of ~100 once multi-window aggregation was exercised.
 
     Args:
-        percentages (List[float | None]): List of percentage returns
-            (as whole numbers, e.g., 10 for 10%).
+        percentages (List[float | None]): List of period returns expressed
+            as decimals (e.g. ``0.10`` for 10%).
 
     Returns:
-        float | None: The compounded percentage return, or None if no
-            valid percentages.
+        float | None: The compounded return as a decimal, or ``None`` if
+            no valid percentages.
     """
     valid_percentages = [p for p in percentages if p is not None]
     if not valid_percentages:
         return None
 
-    # Convert percentages to decimals, compound, then convert back
     compounded = 1.0
     for pct in valid_percentages:
-        compounded *= (1 + pct / 100)
+        compounded *= (1 + pct)
 
-    # Convert back to percentage
-    return (compounded - 1) * 100
+    return compounded - 1
 
 
 def combine_backtests(backtests):
@@ -173,9 +177,13 @@ def generate_backtest_summary_metrics(
         b.total_net_gain for b in valid_metrics
         if b.total_net_gain is not None
     )
+    # B1/B2 fix (issue #511): per-run ``total_loss`` is now the gross
+    # loss magnitude, so the aggregate is simply the sum of per-run
+    # ``total_loss`` (equivalent to ``sum(gross_loss)``). Both per-run
+    # and aggregate use the same unit (positive currency).
     total_loss = sum(
-        b.gross_loss for b in valid_metrics
-        if b.gross_loss is not None
+        b.total_loss for b in valid_metrics
+        if b.total_loss is not None
     )
     total_growth = sum(
         b.total_growth for b in valid_metrics
@@ -184,13 +192,24 @@ def generate_backtest_summary_metrics(
 
     # === PERCENTAGE RETURNS (compounded, not summed) ===
     # Compound returns: (1 + r1) * (1 + r2) * ... - 1
-    # For percentages stored as whole numbers (e.g., 10 for 10%)
+    # All percentages are stored as decimals (e.g. 0.10 for 10%).
     total_net_gain_percentage = _compound_percentage_returns(
         [b.total_net_gain_percentage for b in valid_metrics]
     )
-    total_loss_percentage = _compound_percentage_returns(
-        [b.total_loss_percentage for b in valid_metrics]
-    )
+    # ``total_loss`` is a non-multiplicative magnitude (it does not
+    # compound across windows). Express the aggregate as the sum of
+    # gross losses divided by the sum of initial capital across
+    # windows, which keeps the unit (decimal fraction) consistent
+    # with the per-run definition. See issue #511 (B2).
+    total_initial_value = 0.0
+    for b in valid_metrics:
+        iv = getattr(b, "initial_unallocated", None)
+        if isinstance(iv, (int, float)) and iv > 0:
+            total_initial_value += iv
+    if total_initial_value > 0 and total_loss is not None:
+        total_loss_percentage = total_loss / total_initial_value
+    else:
+        total_loss_percentage = None
     total_growth_percentage = _compound_percentage_returns(
         [b.total_growth_percentage for b in valid_metrics]
     )

investing_algorithm_framework/services/metrics/generate.py

@@ -21,7 +21,7 @@
     get_average_monthly_return, get_average_monthly_return_winning_months, \
     get_average_monthly_return_losing_months, get_cumulative_return, \
     get_cumulative_return_series
-from .returns import get_total_return, get_final_value, get_total_loss, \
+from .returns import get_total_return, get_final_value, \
     get_total_growth
 from .sharpe_ratio import get_sharpe_ratio, get_rolling_sharpe_ratio
 from .sortino_ratio import get_sortino_ratio
@@ -232,6 +232,11 @@ def create_backtest_metrics(
     backtest_metrics = BacktestMetrics(
         backtest_start_date=backtest_run.backtest_start_date,
         backtest_end_date=backtest_run.backtest_end_date,
+        backtest_date_range_name=(
+            backtest_run.backtest_date_range_name or ""
+        ),
+        trading_symbol=backtest_run.trading_symbol or "",
+        initial_unallocated=backtest_run.initial_unallocated or 0.0,
     )
 
     def safe_set(metric_name, func, *args, index=None):
@@ -268,11 +273,26 @@ def safe_set(metric_name, func, *args, index=None):
 
     if "total_loss" in metrics or "total_loss_percentage" in metrics:
         try:
-            total_loss = get_total_loss(backtest_run.portfolio_snapshots)
+            # B1 fix (issue #511): "total_loss" is now the **gross loss**
+            # (sum of P&L of all losing trades, expressed as a non-negative
+            # magnitude) rather than the snapshot net-return clamped at
+            # zero. The latter was indistinguishable from
+            # ``total_net_gain`` whenever the period was unprofitable and
+            # always 0 otherwise. ``total_loss_percentage`` is the gross
+            # loss expressed as a fraction of the initial unallocated
+            # capital (decimal, e.g. ``0.05`` for a 5% loss magnitude).
+            gross_loss_value = get_gross_loss(backtest_run.trades)
+            initial_value = backtest_run.initial_unallocated or 0.0
+
             if "total_loss" in metrics:
-                backtest_metrics.total_loss = total_loss[0]
+                backtest_metrics.total_loss = gross_loss_value
             if "total_loss_percentage" in metrics:
-                backtest_metrics.total_loss_percentage = total_loss[1]
+                if initial_value > 0:
+                    backtest_metrics.total_loss_percentage = (
+                        gross_loss_value / initial_value
+                    )
+                else:
+                    backtest_metrics.total_loss_percentage = 0.0
         except OperationalException as e:
             logger.warning(f"total_loss failed: {e}")
 

investing_algorithm_framework/services/metrics/returns.py

@@ -302,10 +302,19 @@ def get_total_loss(
     snapshots: List[PortfolioSnapshot]
 ) -> Tuple[float, float]:
     """
-    Calculate the total loss from portfolio snapshots.
-
-    The total loss is calculated as the percentage change in portfolio value
-    from the first snapshot to the last snapshot, only if there is a loss.
+    Legacy helper — net portfolio change clamped at zero when positive.
+
+    .. deprecated::
+        This is **not** a real loss metric. It is the net portfolio
+        return clamped to zero whenever the period was profitable, which
+        means it is identical to ``total_net_gain`` whenever the period
+        is unprofitable and ``0.0`` otherwise. It is no longer used to
+        populate ``BacktestMetrics.total_loss`` — that field now holds
+        the **gross loss** (sum of P&L of all losing trades). See issue
+        #511 (B1). This function is kept for backwards compatibility and
+        will be removed in a future release; prefer
+        :func:`get_gross_loss` from
+        ``investing_algorithm_framework.services.metrics.profit_factor``.
 
     Args:
         snapshots (List[PortfolioSnapshot]): List of portfolio snapshots.