Skip to content

feat: add minute/tick level support for long format backtest#5

Open
Yvictor wants to merge 3 commits into
masterfrom
feature/minute-tick-level-support
Open

feat: add minute/tick level support for long format backtest#5
Yvictor wants to merge 3 commits into
masterfrom
feature/minute-tick-level-support

Conversation

@Yvictor

@Yvictor Yvictor commented Feb 1, 2026

Copy link
Copy Markdown
Owner

Summary

  • Migrate internal date types from i32 (days) to i64 (milliseconds) for unified timestamp handling
  • Add DateMode enum to track input type (Date/Datetime) and preserve it in output
  • Add ResampleFreq::Interval(u32) for sub-daily resampling support
  • Support new resample formats: H (hourly), nT (n minutes), nS (n seconds)

Key Changes

File Change
btcore/src/tracker.rs TradeRecord, BacktestResult, StockOperations: i32 → i64
btcore/src/simulation/long.rs Add Interval variant, crossed_interval_boundary
polars_backtest/src/lib.rs DateMode enum, Datetime support, String auto-cast
polars_backtest/src/ffi_convert.rs Add polars_i64_to_arrow
polars_backtest/python/.../namespace.py Add interval validation

Backward Compatibility

  • Date input → Date output (preserved)
  • All existing tests pass (101 Rust, 120 Python)

New Features

# Hourly resampling
df.bt.backtest(date="datetime", resample="H", ...)

# 5-minute resampling  
df.bt.backtest(date="datetime", resample="5T", ...)

# 30-second resampling
df.bt.backtest(date="datetime", resample="30S", ...)

Test plan

  • All 101 Rust tests pass (just test-rust)
  • All 120 Python tests pass (just test)
  • Date input preserves Date output type
  • Datetime input preserves Datetime output type

- Migrate all date types from i32 to i64 millisecond timestamps
- Add DateMode enum to preserve input type (Date/Datetime) in output
- Add ResampleFreq::Interval variant for sub-daily resampling (H, nT, nS)
- Support Datetime input with automatic type preservation
- Support String date columns with auto-cast to Date
- Add interval validation in Python API (_validate_resample)
- Update crossed_interval_boundary for sub-daily boundary detection

Breaking change: Internal date representation changed from i32 days to i64 ms
Backward compatible: Date input still produces Date output
…resample validation

- Update tracker.rs documentation: Date type is now i64 milliseconds, not i32
- Fix Python _INTERVAL_PATTERN regex to allow formats without number prefix
  (e.g., "H", "T", "S") to match Rust parse_interval behavior
@Yvictor

Yvictor commented Feb 2, 2026

Copy link
Copy Markdown
Owner Author

PR #5 深度 Review: feat: add minute/tick level support for long format backtest

PR 概要

目標: 將 polars_backtest 的 Long Format API 從只支援日級 (daily) 擴展到支援任意時間粒度 (hourly/minute/tick)

變更統計: +979/-371 行, 6 個檔案


架構設計評估

核心策略: 統一用 i64 毫秒時間戳

優點:

  1. 統一表示: Date 和 Datetime 都用 i64 毫秒表示,邏輯一致
  2. 類型透明性: 輸入什麼類型,輸出就是什麼類型 (DateMode enum 追蹤)
  3. 向後兼容: 現有 Date 輸入仍產生 Date 輸出
  4. 效能: i64 在現代 CPU 上與 i32 效率相當,不會有顯著效能影響

設計亮點:

  • DateMode enum 巧妙地追蹤輸入類型,確保輸出類型對應
  • 保留現有 _i32 日曆邊界檢測函數,用於月末/週末等檢測
  • 新增 crossed_interval_boundary 用於 sub-daily 邊界檢測

檔案層級 Review

1. btcore/src/simulation/long.rs (+354/-194)

正面評價:

  • ResampleFreq::Interval(u32) 設計合理,用秒數表示間隔
  • parse_interval 實作完整,支援 H/T/S/min 等格式
  • crossed_interval_boundary 簡潔高效
  • ms_to_days 轉換函數用於複用現有日曆邊界檢測

需要注意的問題:

Issue #1: ResampleOffset 的 milliseconds 欄位應該是 i64 而非潛在溢位

pub struct ResampleOffset {
    pub milliseconds: i64,  // 正確使用 i64
}

✅ 已正確處理

Issue #2: 秒級轉換可能的精度問題

fn crossed_interval_boundary(prev_ms: i64, curr_ms: i64, interval_secs: u32) -> bool {
    let interval_ms = interval_secs as i64 * 1000;
    prev_ms / interval_ms != curr_ms / interval_ms
}
  • 此實作使用整數除法檢測邊界,正確
  • interval_secs 使用 u32,最大 ~136 年間隔,足夠

Issue #3: Interval 邊界在 get_all_period_boundaries 中沒有對應處理

ResampleFreq::Interval(_) => {
    // Interval boundaries are handled separately using timestamps
    // No day-based boundaries for sub-day intervals
}
  • 這是正確的設計決定,但需要確認在 offset > 0 時的行為
  • 目前 offset 與 Interval 結合使用可能有未定義行為

建議: 確認 Interval + offset 的組合是否需要支援,如果不支援應加入驗證


2. btcore/src/tracker.rs (+117/-66)

變更摘要:

  • TradeRecord: 所有日期欄位 i32 → i64
  • StockOperations: weight_date, next_weight_date → i64
  • period 欄位: i32 → i64

正面評價:

  • 保持 holding_days() 向後兼容,同時新增 holding_ms()
  • NoopSymbolTracker 類型別名正確更新

需要注意的問題:

Issue #4: TradeTracker trait 文檔未更新

/// - Long format: `Key=String`, `Date=i32`, `Record=TradeRecord`

應更新為:

/// - Long format: `Key=String`, `Date=i64`, `Record=TradeRecord`

3. polars_backtest/src/lib.rs (+276/-100)

DateMode 實作評估:

enum DateMode {
    Date,
    DatetimeMicroseconds,
    DatetimeMilliseconds,
    DatetimeNanoseconds,
}

優點:

  • 完整覆蓋 Polars 的時間類型
  • from_msto_dtype 轉換正確

Issue #5: Datetime 時區處理

DataType::Datetime(time_unit, _tz) => { ... }
  • 目前忽略時區 (_tz)
  • 對於大多數 backtest 場景足夠,但如果輸入帶時區,輸出會丟失時區資訊

建議: 考慮在 DateMode 中保留時區資訊


4. polars_backtest/python/polars_backtest/namespace.py (+38/-10)

新增的 resample 驗證:

_FIXED_RESAMPLE = frozenset({
    None, "D", "W", "W-MON", ..., "H",
})
_INTERVAL_PATTERN = re.compile(r"^(\d+)(H|h|T|t|S|s|min)$")

Issue #6: Python 驗證與 Rust 解析不完全一致

  • Python 驗證: ^(\d+)(H|h|T|t|S|s|min)$ - 需要至少一位數字
  • Rust 解析: 支援純 "H", "T", "S" (無數字前綴)
if num_part.is_empty() {
    return Some(Self::Interval(3600)); // "H" alone = 1 hour
}

建議: 統一 Python regex 為 ^(\d*)(H|h|T|t|S|s|min)$ 允許空數字


5. polars_backtest/src/ffi_convert.rs (+17/-1)

新增 polars_i64_to_arrow 函數,與現有 i32 版本一致,正確實作。


測試覆蓋評估

根據 PR 描述:

  • ✅ 101 Rust 測試通過
  • ✅ 120 Python 測試通過
  • ✅ Date 輸入 → Date 輸出
  • ✅ Datetime 輸入 → Datetime 輸出

建議新增測試案例:

  1. 跨時區 Datetime 處理
  2. Interval + offset 組合行為
  3. 極端 interval 值 (如 1 秒)
  4. 跨日期的 sub-daily resampling

效能影響評估

  1. 記憶體: i64 vs i32 增加一倍時間戳記憶體使用,但對整體影響微小
  2. 計算: ms_to_days 轉換為額外除法操作,但被 inline 優化
  3. FFI: 新增 i64 轉換與 i32 效能相當

結論: 效能影響可忽略


潛在風險

高風險:

  1. Breaking Change: 雖然 API 向後兼容,但 internal 類型從 i32 改為 i64,任何依賴 btcore 內部類型的外部程式碼會 break

中風險:

  1. Interval + Offset 組合: 未完整測試
  2. 時區處理: 輸出會丟失時區資訊

低風險:

  1. Python 驗證不一致: 可能導致 Rust 支援但 Python 拒絕的格式

改進建議

必須修復 (Blocking):

  1. 更新 TradeTracker trait 的文檔 (Issue feat: add CI and release workflows #4)

建議修復 (Non-blocking):

  1. 統一 Python/Rust 的 resample 格式驗證 (Issue #6)
  2. 考慮 Datetime 時區保留 (Issue feat: add minute/tick level support for long format backtest #5)
  3. 新增 Interval + offset 的測試或明確不支援

文檔改進:

  1. PLAN.md 不應該被 commit 到 master (這是實作計畫文件)

結論

這是一個高品質的 PR,架構設計合理,實作完整。核心設計決定(統一用 i64 毫秒時間戳)是正確的選擇,既支援了新功能又保持了向後兼容。

建議: 修復 Issue #4 (文檔更新) 後可以合併。其他 issues 可以作為 follow-up 處理。

整體評分: ⭐⭐⭐⭐ (4/5) - 優秀的實作,小幅改進空間

@Yvictor

Yvictor commented Feb 2, 2026

Copy link
Copy Markdown
Owner Author

🔍 深度可靠性 Review (Senior Engineer Perspective)

本 review 從資深軟體工程師角度,以高可靠性標準檢視程式碼。


🚨 Critical Issues

Issue #7: crossed_interval_boundary 潛在 Division by Zero

// btcore/src/simulation/long.rs:1446-1448
fn crossed_interval_boundary(prev_ms: i64, curr_ms: i64, interval_secs: u32) -> bool {
    let interval_ms = interval_secs as i64 * 1000;
    prev_ms / interval_ms != curr_ms / interval_ms  // ⚠️ Division by zero if interval_secs == 0
}

風險: 如果 interval_secs == 0,程式會 panic。

緩解因素: 目前 parse_interval 不會產生 Interval(0),最小值是 1。但這是隱式保證,沒有在類型層級強制。

建議修復:

fn crossed_interval_boundary(prev_ms: i64, curr_ms: i64, interval_secs: u32) -> bool {
    if interval_secs == 0 {
        return false;  // or panic with descriptive message
    }
    let interval_ms = interval_secs as i64 * 1000;
    prev_ms / interval_ms != curr_ms / interval_ms
}

或在 ResampleFreq 建構時驗證:

pub fn interval(secs: u32) -> Self {
    assert!(secs > 0, "Interval seconds must be positive");
    Self::Interval(secs)
}

Issue #8: DateMode::from_ms Nanoseconds 溢出風險

// polars_backtest/src/lib.rs:88
DateMode::DatetimeNanoseconds => ms * 1_000_000,  // ms to ns

風險: i64 最大值 ~= 9.2e18,如果 ms > 9.2e12 (~year 292471),乘法會溢出。

實際影響: 極低 (超出實際使用範圍),但為了防禦性程式設計:

建議: 使用 checked_mul 或加入範圍檢查

DateMode::DatetimeNanoseconds => ms.checked_mul(1_000_000)
    .expect("Timestamp overflow when converting to nanoseconds"),

Issue #9: timestamps.last().unwrap() 無防護

// btcore/src/simulation/long.rs:1233
let last_price_timestamp = *timestamps.last().unwrap();

風險: 如果 timestamps 為空,會 panic。

觸發條件: 輸入資料有 rows 但無有效 signal 時可能發生。

建議修復:

let last_price_timestamp = match timestamps.last() {
    Some(&ts) => ts,
    None => return StockOperations::new(),  // Early return for empty case
};

⚠️ Medium Risk Issues

Issue #10: unwrap() 呼叫的隱式不變量

以下 unwrap() 依賴隱式不變量,建議改用 expect() 加入說明:

位置 程式碼 不變量
long.rs:387 current_timestamp.unwrap() is_some() 檢查後
long.rs:657 delayed_rebalances.pop_front().unwrap() front() 成功後
long.rs:838 ohlc_accessors.unwrap() is_some() 檢查後
wide.rs:667, long.rs:2085 snapshot.unwrap() is_continuing 保證存在

建議: 將 unwrap() 改為 expect("invariant: ...") 以便在失敗時提供更好的錯誤訊息。


Issue #11: 時區資訊遺失 (已在初版 review 提及)

DataType::Datetime(time_unit, _tz) => { ... }

輸入的 timezone 被忽略,輸出 DataFrame 會丟失時區資訊。

影響: 對於帶時區的 Datetime 輸入,輸出會變成 naive datetime。

建議: 在 DateMode enum 中儲存時區:

enum DateMode {
    Date,
    DatetimeMicroseconds(Option<Arc<str>>),  // Store timezone
    DatetimeMilliseconds(Option<Arc<str>>),
    DatetimeNanoseconds(Option<Arc<str>>),
}

📊 Edge Case 測試建議

目前測試覆蓋良好,但建議新增以下 edge cases:

  1. 極端 interval 值

    • resample="1S" (每秒 rebalance)
    • 非常大的 interval (如 "86400H" = 10 年)
  2. 邊界時間戳

    • Unix epoch (1970-01-01)
    • Year 2038 問題邊界 (i32 overflow)
    • 負時間戳 (1970 年之前)
  3. 空資料處理

    • 有 rows 但全部 weight 都是 NaN
    • 單一 timestamp 多 symbols 全無有效價格
  4. Datetime 精度測試

    • Nanosecond 精度輸入/輸出往返
    • Microsecond vs Millisecond 精度混合

✅ 正面評價

防禦性程式設計亮點

  1. 價格驗證: is_valid_price(price) 檢查 NaN/Inf
  2. 權重容差: weight.abs() > FLOAT_EPSILON 避免浮點精度問題
  3. Factor 驗證: factor.is_finite() && factor > 0.0
  4. 空結果處理: if n_rows == 0 { return (vec![], vec![]); }

程式碼品質

  1. 良好的註解解釋複雜邏輯 (如 delayed rebalance)
  2. 適當使用 #[inline] 優化熱點函數
  3. 清晰的模組邊界和職責分離

📝 總結

類別 數量 狀態
Critical 3 建議修復
Medium 2 建議改進
文檔 2 ✅ 已修復

整體評估: 程式碼品質良好,主要風險來自防禦性程式設計的缺失。Critical issues 在正常使用下不會觸發,但為了達到高可靠性標準,建議處理。

建議優先順序:

  1. Issue #7 (Division by zero) - 加入防護
  2. Issue #9 (empty timestamps) - 加入 early return
  3. Issue #10 (unwrap → expect) - 改善錯誤訊息
  4. Issue #8, #11 - 低優先,可作為 follow-up

- Issue #7: Add division-by-zero guard in crossed_interval_boundary()
  Returns false if interval_secs == 0 instead of panicking

- Issue #8: Use saturating_mul in DateMode::from_ms() for overflow safety
  Prevents panic on extreme timestamps when converting to us/ns

- Issue #9: Replace unwrap() with expect() for better error messages
  Clarifies invariant when accessing timestamps.last()
@Yvictor

Yvictor commented Feb 2, 2026

Copy link
Copy Markdown
Owner Author

✅ Critical Issues 已修復

Commit 64a7525 修復了深度 review 中提出的 3 個 Critical issues:

Issue #7: Division by Zero Guard

fn crossed_interval_boundary(prev_ms: i64, curr_ms: i64, interval_secs: u32) -> bool {
    if interval_secs == 0 {
        return false;  // 新增防護
    }
    let interval_ms = interval_secs as i64 * 1000;
    prev_ms / interval_ms != curr_ms / interval_ms
}

Issue #8: Overflow Safety

fn from_ms(&self, ms: i64) -> i64 {
    match self {
        DateMode::Date => ms / 86_400_000,
        DateMode::DatetimeMicroseconds => ms.saturating_mul(1_000),  // 改用 saturating_mul
        DateMode::DatetimeMilliseconds => ms,
        DateMode::DatetimeNanoseconds => ms.saturating_mul(1_000_000),  // 改用 saturating_mul
    }
}

Issue #9: Better Error Message

let last_price_timestamp = *timestamps.last()
    .expect("invariant: timestamps is non-empty after is_empty() check");

測試結果:

  • ✅ 101 Rust tests passed
  • ✅ 120 Python tests passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant