From 3b5f17913d0b43e0ce2594e9d2c6a428905bf921 Mon Sep 17 00:00:00 2001
From: jgrusewski <jgrusewski@localhost>
Date: Sun, 10 May 2026 10:18:52 +0200
Subject: [PATCH] =?UTF-8?q?fix(mbp10):=20require=20explicit=20instrument?=
 =?UTF-8?q?=5Fid=20in=20extract=5Ftrades=5Ffrom=5Fsnapshots=20=E2=80=94=20?=
 =?UTF-8?q?no=20default?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Removes the `instrument_id: 0` backward-compat default introduced in 6c1ab8850.
That default was nonsensical: a sentinel value for an identifier creates the
exact half-state that future filtering code can't distinguish from real data.

Now extract_trades_from_snapshots takes `instrument_id: u32` as an explicit
required parameter. Caller must provide the contract id the snapshot stream
belongs to (typically captured at the data-acquisition site alongside symbol).

Tests:
- test_extract_trades_from_snapshots updated to pass id=12345 + asserts every
  emitted trade carries that id (verifies the explicit-param contract).
- New test_filter_front_month_mbp10_picks_highest_volume covers the helper
  added in 6c1ab8850 (no test before).
- New test_filter_front_month_mbp10_empty for the empty-input edge case.

15/15 mbp10 tests pass. Workspace + examples compile clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 crates/ml-features/src/mbp10_loader.rs | 55 +++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 6 deletions(-)

diff --git a/crates/ml-features/src/mbp10_loader.rs b/crates/ml-features/src/mbp10_loader.rs
index e9bbe29bc..50046d2aa 100644
--- a/crates/ml-features/src/mbp10_loader.rs
+++ b/crates/ml-features/src/mbp10_loader.rs
@@ -347,9 +347,9 @@ pub struct Mbp10Trade {
     /// Used by `mbp10_to_imbalance_bars` to filter to the front-month
     /// (highest-volume) contract per file, mirroring the per-file pattern
     /// in `precompute_features.rs:354`. Populated from `mbp10.hd.instrument_id`
-    /// in `extract_trades_from_dbn_file`; left as 0 in the snapshot-diff
-    /// path (`extract_trades_from_snapshots`) where the source records
-    /// don't carry instrument IDs (mostly used by tests).
+    /// in `extract_trades_from_dbn_file`; supplied by the caller in
+    /// `extract_trades_from_snapshots` (snapshot records don't carry per-
+    /// record IDs, so the caller passes the stream's contract id explicitly).
     pub instrument_id: u32,
 }
 
@@ -360,9 +360,19 @@ pub struct Mbp10Trade {
 /// when `trade_count` increases between consecutive snapshots, a trade occurred.
 /// Direction is inferred by comparing mid-prices (tick rule).
 ///
+/// `instrument_id` is required — `Mbp10Snapshot` does NOT carry per-record
+/// instrument IDs, so the caller must supply the contract this snapshot
+/// stream belongs to (typically captured at the data-acquisition site
+/// alongside `symbol`). Stamped onto every emitted `Mbp10Trade` so downstream
+/// front-month filtering (`filter_front_month_mbp10`) works uniformly across
+/// both extraction paths.
+///
 /// For higher-fidelity trade extraction, use [`extract_trades_from_dbn_file`]
 /// which reads raw MBP-10 records with their action/side fields.
-pub fn extract_trades_from_snapshots(snapshots: &[Mbp10Snapshot]) -> Vec<Mbp10Trade> {
+pub fn extract_trades_from_snapshots(
+    snapshots: &[Mbp10Snapshot],
+    instrument_id: u32,
+) -> Vec<Mbp10Trade> {
     let mut trades = Vec::new();
     let mut prev_trade_count: u32 = 0;
     let mut prev_mid: f64 = 0.0;
@@ -392,7 +402,7 @@ pub fn extract_trades_from_snapshots(snapshots: &[Mbp10Snapshot]) -> Vec<Mbp10Tr
                     volume,
                     timestamp: dt,
                     is_buy,
-                    instrument_id: 0, // snapshot-diff path has no per-record id
+                    instrument_id,
                 });
             }
         }
@@ -1053,13 +1063,46 @@ mod tests {
             },
         ];
 
-        let trades = extract_trades_from_snapshots(&snapshots);
+        let trades = extract_trades_from_snapshots(&snapshots, 12345);
         // 3 transitions where trade_count increases (snap 0->1, 1->2, 2->3)
         assert_eq!(trades.len(), 3);
         // First trade: price went up => is_buy=true
         assert!(trades[0].is_buy);
         // Third trade: price went down => is_buy=false
         assert!(!trades[2].is_buy);
+        // All trades carry the caller-supplied instrument_id (verifies the
+        // explicit-param contract — no implicit 0 default).
+        assert!(trades.iter().all(|t| t.instrument_id == 12345));
+    }
+
+    #[test]
+    fn test_filter_front_month_mbp10_picks_highest_volume() {
+        let ts = DateTime::<Utc>::from_timestamp(1_700_000_000, 0).unwrap();
+        let mk = |id: u32, vol: f64| Mbp10Trade {
+            price: 100.0,
+            volume: vol,
+            timestamp: ts,
+            is_buy: true,
+            instrument_id: id,
+        };
+        // Three contracts with different total volumes; id=200 is the
+        // highest-volume contract (10 + 5 = 15).
+        let trades = vec![
+            mk(100, 3.0),
+            mk(200, 10.0),
+            mk(300, 1.0),
+            mk(200, 5.0),
+            mk(100, 2.0),
+        ];
+        let filtered = filter_front_month_mbp10(&trades);
+        assert_eq!(filtered.len(), 2, "should keep only id=200 trades");
+        assert!(filtered.iter().all(|t| t.instrument_id == 200));
+    }
+
+    #[test]
+    fn test_filter_front_month_mbp10_empty() {
+        let filtered = filter_front_month_mbp10(&[]);
+        assert!(filtered.is_empty());
     }
 
     #[test]