[tmva][sofie] Fix Conv/Pool padding & ceil_mode bugs, and harden the SOFIE test suite#22862
Open
guitargeek wants to merge 6 commits into
Open
[tmva][sofie] Fix Conv/Pool padding & ceil_mode bugs, and harden the SOFIE test suite#22862guitargeek wants to merge 6 commits into
guitargeek wants to merge 6 commits into
Conversation
guitargeek
added
bug
in:SOFIE
AI-Assisted
clean build
Test Results 23 files 23 suites 3d 19h 2m 15s ⏱️ Results for commit 5f35bb6. ♻️ This comment has been updated with latest results. |
guitargeek
changed the title
The divisor of an AveragePool window was computed at run time only when count_include_pad == 0 && doPadding, and was the compile-time kernel area otherwise. A window can also be partial because of ceil_mode, which lets the last window overhang the input, so that case was averaged over the full kernel area instead of over the cells the window actually covers. Compute the divisor as the kernel extent clipped to the input ([0, size) for count_include_pad = 0, [-padBegin, size + padEnd) otherwise) whenever there is padding or ceil_mode is set, and keep the constexpr kernel area when every window is full, so ordinary pooling generates the same code as before. This replaces the counter that was incremented in the innermost loop. MaxPool is unaffected. Cells that overhang past the padded input are never counted, matching the ONNX reference implementation and PyTorch. Note that ONNX Runtime differs here for count_include_pad = 1, where it divides by the full kernel area. For the 1D example of the issue (input [1..6], kernel 3, stride 2, ceil_mode 1) the generated code now returns [2, 4, 5.5] instead of [2, 4, 3.6667]. Adds AveragePool tests for ceil_mode in 1D/2D/3D, for ceil_mode combined with count_include_pad, and for pads combined with count_include_pad. The four ceil_mode ones fail without this fix. Closes root-project#22523 🤖 Done with the help of AI.
With ceil_mode = 1 the rounding up of the output dimension can add a sliding window that starts past the end of the input, i.e. entirely inside the right padding or in the overhang region. ONNX ignores such a window, but SOFIE emitted it, so both the inferred output shape and the generated loop produced one pooling window too many along the affected dimension. For example an input of 6 with kernel 2, stride 3 and ceil_mode = 1 gave 3 outputs instead of 2, and a 5x5 input with kernel 2, stride 2, pads 1,1,1,1 and ceil_mode = 1 gave 4x4 instead of 3x3. Clip the output dimension to the number of valid window starts in ShapeInference and clip the loop upper bounds in Generate() accordingly. Both are done only when ceil_mode is set; in floor mode the clipping is a no-op for any pad smaller than the kernel, so ordinary pooling is unaffected. This concerns MaxPool as well as AveragePool. Verified against onnxruntime over a sweep of 1760 1D pooling configurations (sizes, kernels, strides, begin/end pads, ceil_mode, both pool modes) with no remaining shape mismatch. Adds AveragePool and MaxPool tests for a ceil_mode window overhanging the input and for ceil_mode combined with pads. All four fail without this fix. 🤖 Done with the help of AI.
Several SOFIE ONNX tests had reference results that a broken
implementation would still pass:
- LinearWithLeakyRelu compared against a tolerance of 1, while the
outputs span [-0.23, 1.33] with a mean magnitude of ~0.3. Returning
all zeros passed 23 of the 24 elements. The test passes with the
DEFAULT_TOLERANCE of 1e-3 that every other test uses.
- Concat0D checked "expected_output.size() == expected_output.size()"
and never looked at the output size, so an empty output passed
silently and an oversized one read past the reference array. Use the
expectNear() helper, which asserts on the size first.
- Greater and LessOrEqual shared an input pair that happens to be
non-discriminating for exactly those two operators: the references
were all-false and all-true respectively, so a constant
implementation passed. Adjust the second operand to mix both
results.
- The Sub reference was {1.0, 1.0}, which an implementation swapping
the operands or returning a constant would also produce. Use
asymmetric operands giving distinct results.
Also stop passing Einsum_dotprod's weight file to the Einsum_3 session.
It is harmless today because Einsum_3 declares no initializers, so the
file is never read, but it would silently load the wrong weights as
soon as that changes.
🤖 Done with the help of AI.
About 30 tests re-implemented what expectNear() and expectEqual()
already do, and did it less safely: they checked the output size with
EXPECT_EQ rather than ASSERT_EQ, and then bounded the comparison loop
by output.size(). A model returning more values than the reference
array holds therefore read past the end of that array instead of
failing on the size mismatch. The helpers assert on the size first and
report the offending index in the failure message.
Add expectNear() and expectEqual() overloads for std::vector<std::vector<T>>
so the multi-output models (Split, Clip) can use them too, and drop the
seven local "constexpr float TOLERANCE = DEFAULT_TOLERANCE" aliases.
Along the way:
- ComparisonBroadcast indexed output[2] without ever checking that the
model returned three tensors. Assert the count first.
- Softplus keeps its bespoke branch logic, but its size check becomes
an ASSERT_EQ, since the loop indexes the input tensor as well.
- The Einsum tests compared floats with EXPECT_EQ against a literal
size. They now use expectNear() with the DEFAULT_TOLERANCE the rest
of the file uses; the expected values are small exact integers, so
nothing meaningful is lost.
No change in coverage: all 153 tests still pass, and perturbing a
reference value still makes the corresponding test fail.
🤖 Done with the help of AI.
The six generators used by TestSofieModels were copy-pasted from one
another: Conv1d and Conv2d differed in about 20 of their 186 lines. The
command-line parsing, the ONNX export and the writing of the expected
output were identical in all of them, so move those to a new
ModelGeneratorUtils.py and keep only the network and the input tensor in
each generator.
The duplication had already caused the copy to drift from the original,
and the --bn / --maxpool / --avgpool paths were broken in three of the
four convolution generators:
- Conv1d used BatchNorm2d, MaxPool2d and AvgPool2d. --bn failed with
"expected 4D input (got 3D input)"; --maxpool did not fail but
pooled over the channel dimension, since MaxPool2d accepts a 3D
tensor as an unbatched input.
- Conv3d likewise used the 2d layers, and failed the same way.
- ConvTrans2d had the layers commented out in __init__ while forward()
still referenced self.bn1, so --bn raised an AttributeError.
Use the layer matching each generator's dimensionality. This is not
covered today, since only Conv2d and Linear are exercised with those
flags (Conv2d_BNORM_B5, Conv2d_MAXPOOL_B2, Conv2d_AVGPOOL_B2 and
Linear_BNORM_B8), but the flags are advertised by every generator and
are useful when debugging a model by hand.
The "new ONNX exporter does not work for batchnorm" guard existed only
in Conv2d and Linear, and was missing from the generators it was added
after. It now lives in the shared export_onnx().
Also fix RecurrentModelGenerator, where --v assigned to a local instead
of the module-level "verbose" flag it was meant to set.
All 14 TestSofieModels tests still pass, and every --bn / --maxpool /
--avgpool combination now runs for all the generators.
🤖 Done with the help of AI.
UTILITY::Im2col and Im2col_3d took a single pad per axis and used it both as the offset of the first window and, doubled, in the output size: output_h = (height + 2 * pad_h - (dilation_h * (kernel_h - 1) + 1)) / stride_h + 1 ONNX allows the padding at the beginning and at the end of an axis to differ, through the "pads" attribute and through SAME_UPPER/SAME_LOWER whenever the total padding is odd, which happens for an even kernel size. ROperator_Conv's shape inference already handles that correctly with fAttrPads[d] + fAttrPads[d + fDim], and so does ROperator_Pool, but the Im2col call could not express it: GenerateInitCode replaced the two pads by their average and printed TMVA SOFIE Operator Conv: asymmetric padding not supported. Assume an average padding The result was worse than an approximation. The _xcol buffer is sized from the (correct) inferred output shape, while Im2col filled it using the averaged padding, so it wrote a different number of columns than were allocated and the convolution read uninitialized memory. For a 1x1x4x4 input with a 2x2 kernel and SAME_UPPER, SOFIE returned 21 25 20 24 11 14 ... where ONNX gives 10 14 18 10 26 30 ... Take the begin and end pads separately in Im2col and Im2col_3d, use their sum in the output size and the begin pad as the window offset, and pass fAttrPads[d] and fAttrPads[d + fDim] from ROperator_Conv. The unreachable Im2col_3d calls emitted by ROperator_ConvTranspose (they sit behind a "3D Conv Transpose not yet supported" throw) are updated too, so they do not become a latent compile error. Add six models covering the 1d, 2d, 3d and grouped code paths of ROperator_Conv, with SAME_UPPER, SAME_LOWER and explicit "pads". All six fail before this change and pass after it. ConvWithAsymmetricPadding, disabled since it was added, is enabled. It turned out not to be blocked by this bug at all: its pads [1, 0, 1, 0] are symmetric on each axis, so the averaging never triggered. What made it fail is that the model declared an output shape of [1, 1, 5, 5] while the attributes give [1, 1, 4, 2]. Correct the declared shape. Closes root-project#21873 🤖 Done with the help of AI.
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.
Closes #22523
Closes #21873
This PR fixes a set of correctness bugs in the SOFIE
ConvandPooloperators around padding andceil_mode, and in the process substantially hardens the SOFIE ONNX test suite. Several tests turned out to pass even against a broken implementation.Operator fixes
AveragePool divisor for partial
ceil_modewindows (d4d8792, closes [tmva][sofie] AveragePool with ceil_mode divides the partial/overhang window by the full kernel size #22523). The divisor was only computed at run time forcount_include_pad == 0 && doPadding; a window made partial byceil_mode(overhanging the input) was wrongly averaged over the full kernel area. The divisor is now the kernel extent clipped to the input whenever there is padding orceil_mode, and stays theconstexprkernel area when every window is full (so ordinary pooling generates identical code). E.g. input[1..6], kernel 3, stride 2,ceil_mode 1now returns[2, 4, 5.5]instead of[2, 4, 3.6667].Ignore
ceil_modewindows that start past the input (a756e86). Rounding the output dimension up could add a sliding window starting entirely inside the right padding; ONNX drops such a window but SOFIE emitted it, so both the inferred shape and the generated loop had one window too many. Now clipped inShapeInferenceand inGenerate(), only whenceil_modeis set (no-op in floor mode). Verified against onnxruntime over a sweep of 1760 1D pooling configurations. Affects bothMaxPoolandAveragePool.Asymmetric padding in
Im2col(e8c5144, closes [tmva][sofie] Extend Im2col to support asymmetric padding for full ONNX SAME autopad compliance #21873).Conv's shape inference already handled per-end padding (fAttrPads[d] + fAttrPads[d + fDim]), butIm2col/Im2col_3dcollapsed the two pads to their average. Because the_xcolbuffer is sized from the correctly-inferred output shape, this wrote a different number of columns than were allocated and the convolution then read uninitialized memory (e.g.SAME_UPPERon a 1×1×4×4 input with a 2×2 kernel gave garbage).Im2col/Im2col_3dnow take begin/end pads separately. Adds six models covering the 1d/2d/3d/grouped paths withSAME_UPPER,SAME_LOWERand explicitpads, and re-enablesConvWithAsymmetricPadding(which was failing for an unrelated wrong declared-output-shape).Test-suite hardening
Fix tests with non-discriminating reference values (
4518137). Several tests passed against a broken implementation:LinearWithLeakyReluused a tolerance of 1 (all-zeros passed 23/24 elements);Concat0Dcompared its size to itself;Greater/LessOrEqualshared a non-discriminating input pair;Sub's reference was symmetric. All now use discriminating references and the standardDEFAULT_TOLERANCE.Use the comparison helpers instead of hand-rolled loops (
5772ee2). ~30 tests re-implementedexpectNear()/expectEqual()less safely: checking size withEXPECT_EQand then bounding the loop byoutput.size(), so an oversized output read past the reference array instead of failing. Addsvector<vector<T>>overloads for the multi-output models (Split,Clip). No coverage change: all 153 tests still pass.Share the common code of the PyTorch model generators (
e97ccc2). The six generators were copy-pasted and had drifted; the--bn/--maxpool/--avgpoolpaths were broken in three of the four convolution generators (e.g.Conv1dusingBatchNorm2d/MaxPool2d). Command-line parsing, ONNX export and expected-output writing move to a sharedModelGeneratorUtils.py, each generator keeps only its network and input tensor, and the dimensionality-correct layers are used throughout.Testing
All SOFIE tests pass. The new operator tests each fail before their respective fix and pass after, and perturbing any reference value still fails the corresponding test.