From 236e1665cff4f74bd5483bc5ef597f3c4df3eb34 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Mon, 23 Feb 2026 15:17:58 +0100 Subject: [PATCH] fix(ml): address code review findings - Remove panic!() calls from test_futures_baseline_micro_mapping, use map()+Some() pattern consistent with rest of test suite - Make DatasetSpec::from_universe() accept a name parameter instead of hardcoding "futures-baseline" - Add doc comment to UniverseConfigMeta explaining dead_code fields Co-Authored-By: Claude Opus 4.6 --- ml/src/asset_selection/mod.rs | 38 ++++++----------- ml/src/data_pipeline/mod.rs | 77 +++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 25 deletions(-) diff --git a/ml/src/asset_selection/mod.rs b/ml/src/asset_selection/mod.rs index 02d16ea4f..8abebc510 100644 --- a/ml/src/asset_selection/mod.rs +++ b/ml/src/asset_selection/mod.rs @@ -73,6 +73,9 @@ struct UniverseConfigFile { symbols: Vec, } +/// Parsed metadata from the `[universe]` section of the TOML config. +/// Fields are currently used only for validation/documentation during parsing; +/// they will feed into `DatasetSpec` construction once download automation lands. #[derive(Deserialize)] struct UniverseConfigMeta { #[allow(dead_code)] @@ -415,38 +418,23 @@ mod tests { #[test] fn test_futures_baseline_micro_mapping() { let universe = AssetUniverse::futures_baseline(); - let es = universe.find("ES.FUT"); - assert!(es.is_some()); - assert_eq!( - es.unwrap_or_else(|| panic!("ES.FUT not found")) - .execution_symbol(), - "MES.FUT" - ); - let nq = universe.find("NQ.FUT"); - assert!(nq.is_some()); assert_eq!( - nq.unwrap_or_else(|| panic!("NQ.FUT not found")) - .execution_symbol(), - "MNQ.FUT" + universe.find("ES.FUT").map(|a| a.execution_symbol()), + Some("MES.FUT") + ); + assert_eq!( + universe.find("NQ.FUT").map(|a| a.execution_symbol()), + Some("MNQ.FUT") ); - - let zn = universe.find("ZN.FUT"); - assert!(zn.is_some()); // ZN has no micro -- executes as ZN.FUT assert_eq!( - zn.unwrap_or_else(|| panic!("ZN.FUT not found")) - .execution_symbol(), - "ZN.FUT" + universe.find("ZN.FUT").map(|a| a.execution_symbol()), + Some("ZN.FUT") ); - - let six_e = universe.find("6E.FUT"); - assert!(six_e.is_some()); assert_eq!( - six_e - .unwrap_or_else(|| panic!("6E.FUT not found")) - .execution_symbol(), - "M6E.FUT" + universe.find("6E.FUT").map(|a| a.execution_symbol()), + Some("M6E.FUT") ); } diff --git a/ml/src/data_pipeline/mod.rs b/ml/src/data_pipeline/mod.rs index fcce3f44f..4288b2228 100644 --- a/ml/src/data_pipeline/mod.rs +++ b/ml/src/data_pipeline/mod.rs @@ -166,6 +166,35 @@ impl DatasetSpec { } } + /// Create a spec from an asset universe and dataset mode. + /// + /// Only includes enabled assets from the universe. + #[must_use] + pub fn from_universe( + name: &str, + universe: &crate::asset_selection::AssetUniverse, + mode: DatasetMode, + ) -> Self { + let symbols = universe + .eligible() + .iter() + .map(|a| SymbolSpec { + symbol: a.symbol.clone(), + exchange: a.exchange.clone(), + asset_class: a.asset_class.clone(), + }) + .collect(); + + Self { + name: format!("{name}-{mode:?}"), + symbols, + date_range: mode.to_date_range(), + bar_size: BarSize::OneMinute, + split_ratio: SplitRatio::default(), + warmup_bars: 50, + } + } + /// Estimate total bars across all symbols pub fn estimated_total_bars(&self) -> usize { let trading_days = (self.date_range.calendar_days() as f64 * 252.0 / 365.0) as usize; @@ -267,4 +296,52 @@ mod tests { let as_future = crate::asset_selection::AssetClass::Future; assert_eq!(dp_future, as_future); } + + #[test] + fn test_dataset_spec_from_universe() { + let universe = crate::asset_selection::AssetUniverse::futures_baseline(); + let spec = DatasetSpec::from_universe("futures-baseline", &universe, DatasetMode::Dev); + + assert_eq!(spec.symbols.len(), 4); + assert!(spec.name.contains("Dev")); + assert_eq!(spec.bar_size, BarSize::OneMinute); + assert!(spec.split_ratio.is_valid()); + + // Verify symbols match universe + let symbol_names: Vec<&str> = spec.symbols.iter().map(|s| s.symbol.as_str()).collect(); + assert!(symbol_names.contains(&"ES.FUT")); + assert!(symbol_names.contains(&"NQ.FUT")); + assert!(symbol_names.contains(&"ZN.FUT")); + assert!(symbol_names.contains(&"6E.FUT")); + + // All should be Future class + for sym in &spec.symbols { + assert_eq!(sym.asset_class, AssetClass::Future); + } + } + + #[test] + fn test_dataset_spec_from_universe_modes() { + let universe = crate::asset_selection::AssetUniverse::futures_baseline(); + + let dev = DatasetSpec::from_universe("futures-baseline", &universe, DatasetMode::Dev); + let backtest = DatasetSpec::from_universe("futures-baseline", &universe, DatasetMode::Backtest); + let full = DatasetSpec::from_universe("futures-baseline", &universe, DatasetMode::Full); + + // Dev < Backtest < Full in estimated bars + assert!(dev.estimated_total_bars() < backtest.estimated_total_bars()); + assert!(backtest.estimated_total_bars() < full.estimated_total_bars()); + } + + #[test] + fn test_dataset_spec_from_universe_disabled_filtered() { + let mut universe = crate::asset_selection::AssetUniverse::futures_baseline(); + // Disable one asset + if let Some(first) = universe.assets.get_mut(0) { + first.enabled = false; + } + let spec = DatasetSpec::from_universe("futures-baseline", &universe, DatasetMode::Dev); + // Only 3 symbols because one was disabled + assert_eq!(spec.symbols.len(), 3); + } }