fix(sim): two rules of one kind on one asset get a row each (#829)

keel/sim/report.py

@@ -100,6 +100,7 @@
     "edge_table",
     "group_trades_by_class",
     "render_markdown",
+    "rule_keys",
 ]
 
 # G1: an asset's ONE_HOUR coverage below this many bars (~166 days) is excluded from the pooled
@@ -121,6 +122,37 @@
 # `edge_table`'s pooled entry key.
 POOLED_KEY = "__pooled__"
 
+
+def rule_keys(rules: list[Rule]) -> list[str]:
+    """One row key per rule in `rules`, positionally -- unique across the list (#829).
+
+    The key is `"{rule.name}:{asset}"`, which is what every existing reader of these tables
+    expects, and it stays exactly that whenever it is unique. Only rules that SHARE it -- two
+    rules of one kind on one asset, e.g. the deployment's $50 and $5 BTC DCA rows -- are told
+    apart, by their `rules.id` (`"dca#19:BTC"`), or by their 1-based position among the
+    colliding rules when a hand-built rule has no id. Before this, the later rule overwrote the
+    earlier one's row, and `group_trades_by_class`, looking both up by the shared key, counted
+    the survivor twice and the other not at all.
+
+    `edge_table`, `accumulation_table` and `group_trades_by_class` must each be handed the SAME
+    `rules` list for their keys to agree -- `simulate` passes one list to all three.
+    """
+    bases = [f"{rule.name}:{_asset(rule.product_id)}" for rule in rules]
+    counts: dict[str, int] = {}
+    for base in bases:
+        counts[base] = counts.get(base, 0) + 1
+    seen: dict[str, int] = {}
+    keys: list[str] = []
+    for rule, base in zip(rules, bases, strict=True):
+        if counts[base] == 1:
+            keys.append(base)
+            continue
+        seen[base] = seen.get(base, 0) + 1
+        tag = rule.rule_id if rule.rule_id is not None else seen[base]
+        keys.append(f"{rule.name}#{tag}:{_asset(rule.product_id)}")
+    return keys
+
+
 # A coverage "first_ts starts this much later than requested" margin (30 days) before flagging
 # partial history -- small pagination/inception-boundary slop shouldn't itself be a gap.
 _PARTIAL_HISTORY_MARGIN_SEC = 30 * 86400
@@ -166,9 +198,10 @@ def edge_table(
     convention). A rule is backtested on the series for ITS OWN trading timeframe
     (`_rule_trading_tf` -- `ONE_HOUR` for the hourly rules, `ONE_DAY` for the daily-native
     `TurtleBreakout`), with the `ONE_HOUR` series passed as `finer_candles` for intrabar
-    stop/target resolution when the rule's own timeframe is coarser. Results are keyed
-    `"{rule.name}:{asset}"` (not bare `rule.name`) so two rules of the same kind bound to
-    different assets don't collide. `POOLED_KEY` (`"__pooled__"`) holds
+    stop/target resolution when the rule's own timeframe is coarser. Results are keyed by
+    `rule_keys` -- `"{rule.name}:{asset}"`, so two rules of the same kind on different assets
+    don't collide, and disambiguated by rule id when two share one asset (#829).
+    `POOLED_KEY` (`"__pooled__"`) holds
     `strategy.stats.summarize()` over every rule's trades in EXIT-TIME order
     (`_chronological`) -- the pooled sample `build_verdict`'s G2 gate is checked against, and
     whose drawdown must be a path that happened, not one rule's history followed by the next's.
@@ -191,7 +224,7 @@ def edge_table(
     results: dict[str, BacktestResult] = {}
     pooled_trades = []
 
-    for rule in rules:
+    for rule, key in zip(rules, rule_keys(rules), strict=True):
         if rule.accumulates:
             continue
         asset = _asset(rule.product_id)
@@ -207,7 +240,7 @@ def edge_table(
             slippage_pct=slippage_pct,
             slippage_by_product=slippage_by_product,
         )
-        results[f"{rule.name}:{asset}"] = result
+        results[key] = result
         pooled_trades.extend(result.trades)
 
     results[POOLED_KEY] = summarize(_chronological(pooled_trades))
@@ -222,7 +255,8 @@ def accumulation_table(
     slippage_by_product: Callable[[str], Decimal] | None = None,
 ) -> dict[str, DcaSleeve]:
     """The edge pass for ACCUMULATING rules (`Rule.accumulates` -- `Dca`), keyed like
-    `edge_table` (`"{rule.name}:{asset}"`) but never mixed into it (#821).
+    `edge_table` (`rule_keys`: `"{rule.name}:{asset}"`, disambiguated on collision) but never
+    mixed into it (#821).
 
     Each rule is driven over its asset's `ONE_DAY` series, one completed day at a time: every
     bar in that series has closed, so `Dca.detect`'s completed-day guard keeps them all, and
@@ -238,7 +272,7 @@ def accumulation_table(
     zero-buy row rather than raising.
     """
     rows: dict[str, DcaSleeve] = {}
-    for rule in rules:
+    for rule, key in zip(rules, rule_keys(rules), strict=True):
         if not rule.accumulates:
             continue
         asset = _asset(rule.product_id)
@@ -271,7 +305,7 @@ def accumulation_table(
             qty += buy_qty
             cost += fill * buy_qty * (Decimal(1) + fee_pct)
         last_close = daily[-1].close if daily else Decimal("0")
-        rows[f"{rule.name}:{asset}"] = DcaSleeve.marked(buys, qty, cost, last_close)
+        rows[key] = DcaSleeve.marked(buys, qty, cost, last_close)
     return rows
 
 
@@ -296,14 +330,15 @@ def group_trades_by_class(
     `build_verdict`'s G2 gate checks against each class's own floor (KB §25.5): a
     low-win/high-R:R trend-follower is judged by the trend floor rather than the global one,
     while other classes keep the canonical floor. `edge` is `edge_table`'s output; its
-    per-rule keys are `"{rule.name}:{asset}"` (the `POOLED_KEY` entry is ignored -- it pools
+    per-rule keys come from `rule_keys(rules)`, so `rules` must be the list `edge_table` was
+    given (the `POOLED_KEY` entry is ignored -- it pools
     across *all* classes and so isn't meaningful per-class). A rule whose edge entry is
     missing is skipped (absent data is a coverage gap, not a crash -- mirrors `edge_table`).
     Each class's pool is summarised in exit-time order, like `edge_table`'s (`_chronological`).
     """
     trades_by_class: dict[str, list[Trade]] = {}
-    for rule in rules:
-        result = edge.get(f"{rule.name}:{_asset(rule.product_id)}")
+    for rule, key in zip(rules, rule_keys(rules), strict=True):
+        result = edge.get(key)
         if result is None:
             continue
         trades_by_class.setdefault(promotion_class_of(rule), []).extend(result.trades)

tests/sim/test_rule_keys.py

@@ -0,0 +1,117 @@
+"""Two rules of one kind on one asset must not share a row (#829).
+
+`edge_table` and `accumulation_table` keyed rows `"{rule.name}:{asset}"`, so a second rule of
+the same kind on the same asset overwrote the first. On the deployment's paper db that hid a
+$50/week BTC DCA behind a $5 one. `group_trades_by_class` looked rules up by the same key, so
+both rules then read the SURVIVOR's trades: one rule counted twice in G2's class pool, the
+other not at all. A key that is unique already stays exactly as it was.
+"""
+
+from __future__ import annotations
+
+from decimal import Decimal
+
+from keel.sim import report
+from keel.sim.report import POOLED_KEY, edge_table, group_trades_by_class
+from keel.strategy.promotion import promotion_class_of
+from keel.strategy.rules.dca import Dca
+from tests.sim.test_dca_sleeve import _cadence_days, _market
+from tests.sim.test_report import _HOUR, _OneShotRule, _winning_series
+
+_ZERO = Decimal("0")
+
+
+def _one_shot(product_id: str, trigger_ts: int, rule_id: int | None) -> _OneShotRule:
+    rule = _OneShotRule(
+        product_id, trigger_ts, entry=Decimal("100"), stop=Decimal("90"), target=Decimal("120")
+    )
+    rule.rule_id = rule_id
+    return rule
+
+
+def _dca(budget: str, rule_id: int | None) -> Dca:
+    rule = Dca("BTC-USD", cadence_days=7, budget_usd=Decimal(budget))
+    rule.rule_id = rule_id
+    return rule
+
+
+def _btc() -> dict:
+    from keel.types import Granularity
+
+    return {"BTC": {Granularity.ONE_HOUR: _winning_series("BTC")}}
+
+
+def test_two_dca_rules_on_one_asset_get_a_row_each_at_their_own_budget() -> None:
+    """The deployment's shape: rule 19 at $50 and rule 20 at $5, both on BTC."""
+    n = len(_cadence_days())
+
+    rows = report.accumulation_table(
+        [_dca("50", 19), _dca("5", 20)], _market(), fee_pct=_ZERO, slippage_pct=_ZERO
+    )
+
+    assert sorted(rows) == ["dca#19:BTC", "dca#20:BTC"]
+    assert (rows["dca#19:BTC"].buys, rows["dca#19:BTC"].cost_usd) == (n, Decimal("50") * n)
+    assert (rows["dca#20:BTC"].buys, rows["dca#20:BTC"].cost_usd) == (n, Decimal("5") * n)
+
+
+def test_two_edge_rules_on_one_asset_get_a_row_each_and_the_pool_counts_both() -> None:
+    fires = _one_shot("BTC-USD", _HOUR, 7)
+    never = _one_shot("BTC-USD", 99 * _HOUR, 8)
+
+    edge = edge_table([fires, never], _btc(), fee_pct=_ZERO, slippage_pct=_ZERO)
+
+    assert sorted(edge) == [POOLED_KEY, "one_shot#7:BTC", "one_shot#8:BTC"]
+    assert edge["one_shot#7:BTC"].n_trades == 1
+    assert edge["one_shot#8:BTC"].n_trades == 0
+    assert edge[POOLED_KEY].n_trades == 1
+
+
+def test_the_class_pool_counts_each_colliding_rule_once() -> None:
+    """Before #829 both rules read the survivor's (the later rule's, zero-trade) result, so
+    G2's class pool held 0 trades while the pooled row held 1."""
+    fires = _one_shot("BTC-USD", _HOUR, 7)
+    never = _one_shot("BTC-USD", 99 * _HOUR, 8)
+    rules = [fires, never]
+
+    edge = edge_table(rules, _btc(), fee_pct=_ZERO, slippage_pct=_ZERO)
+    by_class = group_trades_by_class(edge, rules)
+
+    assert by_class[promotion_class_of(fires)].n_trades == 1
+
+
+def test_a_key_that_is_already_unique_is_unchanged() -> None:
+    btc = _one_shot("BTC-USD", _HOUR, 7)
+    eth = _one_shot("ETH-USD", _HOUR, 8)
+    from keel.types import Granularity
+
+    candles = {
+        "BTC": {Granularity.ONE_HOUR: _winning_series("BTC")},
+        "ETH": {Granularity.ONE_HOUR: _winning_series("ETH")},
+    }
+
+    edge = edge_table([btc, eth], candles, fee_pct=_ZERO, slippage_pct=_ZERO)
+
+    assert sorted(edge) == [POOLED_KEY, "one_shot:BTC", "one_shot:ETH"]
+
+
+def test_colliding_rules_without_an_id_are_still_told_apart() -> None:
+    """A hand-built rule has `rule_id=None`; its position in `rules` disambiguates instead, so
+    no row is ever lost for want of a database id."""
+    first = _one_shot("BTC-USD", _HOUR, None)
+    second = _one_shot("BTC-USD", 99 * _HOUR, None)
+
+    edge = edge_table([first, second], _btc(), fee_pct=_ZERO, slippage_pct=_ZERO)
+
+    assert len([key for key in edge if key != POOLED_KEY]) == 2
+    assert edge[POOLED_KEY].n_trades == 1
+
+
+def test_the_accumulation_section_renders_both_rows() -> None:
+    rows = report.accumulation_table(
+        [_dca("50", 19), _dca("5", 20)], _market(), fee_pct=_ZERO, slippage_pct=_ZERO
+    )
+
+    lines = report._render_accumulation_section(rows)
+
+    data = [line for line in lines if line.startswith("| dca")]
+    assert [line.split("|")[1].strip() for line in data] == ["dca#19:BTC", "dca#20:BTC"]