fix: consume the first tick threshold so features align with bars - #68
Merged
Merged
Conversation
new_from_csv read the first threshold with a peek and left it in the iterator, so it was served again after the first bar closed and the next tick emitted a spurious one-tick bar. Thresholds [10, 20, 30] produced four rows, with every row after the first misaligned against the bars. Take the first threshold out of the iterator, as mlfinlab's generator does. The new test builds 30 synthetic ticks and asserts exactly three rows with average tick sizes 1, 2, 3; it fails with [1, 2, 2, 3] before this change. Fixes #67 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #67.
Problem
MicrostructuralFeaturesGenerator::new_from_csvpeeked at the first tick threshold instead of taking it from the iterator. After the first bar closed the same threshold was served again, so the next tick closed a one-tick bar. Thresholds[10, 20, 30]produced 4 rows, and every row after the first was misaligned with the bars that defined the thresholds.Fix
Take the first threshold out of the iterator (what mlfinlab's generator does with
next()).Evidence
test_feature_generator_emits_one_row_per_tick_threshold: 30 synthetic ticks, thresholds[10, 20, 30], asserts average tick sizes[1, 2, 3]. Onmainit fails with[1, 2, 2, 3].cargo test --workspace: 247 passed / 0 failed / 1 ignored. No existing expectation changed — the existing generator test only asserted non-empty output, which is why this went unnoticed.cargo fmt --checkand clippy (CI flags) clean.Behavior change
Callers get one fewer row (the degenerate second row disappears) and rows now align one-to-one with
tick_num_series.Merge note
#64 puts an
#[allow(clippy::unnecessary_to_owned)]on the initialiser line this PR rewrites, so the second of the two to merge will need a small conflict resolution.🤖 Generated with Claude Code