Fix SplitToSequence with scalar uneven split producing incorrect equal-split output#2858
Fix SplitToSequence with scalar uneven split producing incorrect equal-split output#2858Copilot wants to merge 4 commits into
Conversation
…lit node When split is a scalar that doesn't evenly divide the axis dimension, create an explicit split sizes tensor and pass it to Split, instead of using num_outputs alone (which would produce an equal split). Example: input [1,8400,80] with scalar split=5000 on axis=1 now correctly produces [1,5000,80] and [1,3400,80] instead of [1,4200,80] and [1,4200,80]. Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
|
What is the reason the PR cannot be merged? It's a rathe simple fix. |
|
Thanks for the reminder. I must have slipped |
There was a problem hiding this comment.
Pull request overview
This PR fixes a semantic bug in the constant-folding optimizer where SplitToSequence with an uneven scalar split size could be rewritten into a Split that performs an equal/near-equal split, changing the model’s output shapes/semantics. It updates the rewrite to preserve the intended chunk sizes by materializing an explicit split-size tensor, and adds a regression test to prevent recurrence.
Changes:
- Update
split_to_sequenceconstant-folding to emit an explicitsplitsize tensor for uneven scalar splits. - Keep the existing
num_outputs-only rewrite path for evenly divisible splits. - Add a regression test asserting the generated
Splitcarries explicit split sizes (e.g.,[5000, 3400]) and thatSequenceAtnodes are eliminated.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxscript/optimizer/_constant_folding.py | Adjusts SplitToSequence folding to preserve uneven scalar split semantics by providing explicit split sizes. |
| onnxscript/optimizer/_constant_folding_test.py | Adds regression coverage to ensure uneven scalar splits remain unequal after folding. |
|
@copilot check review comments and fix or rebut |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2858 +/- ##
==========================================
+ Coverage 72.64% 72.65% +0.01%
==========================================
Files 259 259
Lines 31652 31673 +21
Branches 2980 2982 +2
==========================================
+ Hits 22994 23013 +19
- Misses 7649 7650 +1
- Partials 1009 1010 +1 ☔ View full report in Codecov by Sentry. |
… split size, guard test indexing
Addressed all three review comments in 523c2aa:
All 8 split_to_sequence tests pass. |
When optimizing
SplitToSequencewith a scalarsplitthat doesn't evenly divide the axis dimension, the optimizer was emitting aSplitnode with onlynum_outputs, which produces an equal split — silently corrupting the model semantics.Example: Input
[1, 8400, 80]withsplit=5000onaxis=-2should produce[1, 5000, 80]+[1, 3400, 80], but was producing[1, 4200, 80]+[1, 4200, 80].Changes
optimizer/_constant_folding.py— Insplit_to_sequence, when the scalar split doesn't evenly divide the axis dimension (split_dimension_size % split_size != 0), construct an explicit 1-D constant tensor[split_size, ..., remainder]and pass it as thesplitinput toSplit. Per the opset 18 spec,Splitaccepts either thesplitinput or thenum_outputsattribute but not both, sonum_outputsis omitted in this branch. Even splits retain the existingnum_outputs-only path. Additionally, the rewrite now bails out cleanly (returnsNone) when the scalarsplit_sizeis0or negative, instead of raising on invalid input models.optimizer/_constant_folding_test.py— Adds a regression test asserting the generatedSplitnode carries explicit split sizes[5000, 3400]rather than performing an equal split. The test asserts theSplitinput count before indexing so a regression fails with a clear assertion message rather than anIndexError.Original prompt
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.