Repository navigation
fix(execution): stop VWAP emitting negative slice quantities (BUG-15) - #25
Merged
Merged
Conversation
allocated = 0
for i in range(n_slices):
if i == n_slices - 1:
slice_qty = order.quantity - allocated # remainder
else:
slice_qty = max(100, int(order.quantity * volume_profile[i]))
slice_qty = (slice_qty // 100) * 100
allocated += slice_qty
plan.slices.append(ExecutionSlice(
quantity=min(slice_qty, order.quantity - (allocated - slice_qty)),
...))
The lot floor can exceed what is left on a small order, and the clamp meant to
catch it compared against `allocated` *after* it had already been incremented,
so it never bit. A 200-share parent over 10 slices came out as
[100, 100, 0, -100, -200, -300, -400, -500, -600, -700]
summing to -2600. Any consumer executing those slices trades phantom short
quantities. `SmartRouter` routes to VWAP when 0.15 <= quantity/ADV < 0.30, so
an illiquid name with a small order reaches it.
Fix: clamp against what actually remains, stop rather than emit a non-positive
slice, and set `plan.num_slices` to the number of slices actually produced
rather than the number requested. If the order is too small to spread across
lot-sized slices, that is logged rather than silently mis-allocated.
before 200 shares -> [100, 100, 0, -100, ..., -700] sum -2600
after 200 shares -> [100, 100] sum 200
after 10000 shares -> [1500, 1300, 1100, 900, 900, 900, 900, 700, 700, 1100]
Tests (5, parametrized over quantities): slices are positive, lot-sized, and
sum exactly to the parent; a 200-share order is not split into ten slices; and
a plan never exceeds its parent. Verified against the old implementation --
the 200 and 500 cases and the over-split case fail there; 100, 1000 and 10000
pass both ways, because they are large enough not to trip the floor.
1341 passed, 1 skipped.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
P1.
The lot floor can exceed what is left on a small order, and the clamp meant to catch it compares against
allocatedafter it has already been incremented — so it never bit.Measured
[100, 100, 0, -100, -200, -300, -400, -500, -600, -700]Any consumer executing those slices trades phantom short quantities.
SmartRouterroutes to VWAP when0.15 <= quantity/ADV < 0.30, so an illiquid name with a small order reaches this path.[100, 100]— sum 200[1500, 1300, 1100, 900, 900, 900, 900, 700, 700, 1100]— sum 10 000Fix
plan.num_slicesto the number of slices actually produced, not the number requested — otherwise a 200-share order advertises 10 slices when it has 2Tests — 5, parametrized over quantities
Slices are positive, lot-sized, and sum exactly to the parent; a 200-share order is not split into ten slices; a plan never exceeds its parent.
Verified against the old implementation: the 200 and 500 cases and the over-split case fail there; 100, 1000 and 10 000 pass both ways, because they are large enough not to trip the floor. Recorded so the passing cases are not mistaken for coverage of the bug.
1341 passed, 1 skipped.🤖 Generated with Claude Code