Skip to content

[tmva][sofie] Fix LogSoftmax returning -inf for inputs far apart from the maximum - #23551

Closed
aniruddhaadak80 wants to merge 2 commits into
root-project:masterfrom
aniruddhaadak80:fix-sofie-logsoftmax-underflow
Closed

aniruddhaadak80 wants to merge 2 commits into
root-project:masterfrom
aniruddhaadak80:fix-sofie-logsoftmax-underflow

Conversation

@aniruddhaadak80

Copy link
Copy Markdown
Contributor

Problem

ROperator_Softmax computes LogSoftmax as log(softmax(x)). It normalizes
exp(x - max) by the sum and then takes the log of the result. When an input is
more than ~88 below the maximum, exp(x - max) underflows to 0 in float, so
the log returns -inf instead of a large negative number. Values close to the
underflow also lose precision (e.g. -99.98 instead of -100).

For the reproducer from #23547 (x = [[0, 100, 200]], axis = -1):

onnxruntime:    [[-200.0, -100.0, 0.0]]        <- correct
onnx.reference: [[-inf, -99.98309326171875, 0.0]]
SOFIE:          [-inf, -99.98309326171875, 0.0]

Note that onnx.reference is wrong for the same reason, so it cannot be used as
the reference for a test of this case.

Root cause

ROperator_Softmax::Generate() emits, for the contiguous last-axis case:

y_ptr[j] = exp(x_ptr[j] - vmax);
sum += y_ptr[j];
...
y_ptr[j] *= inv_sum;
y_ptr[j] = log(y_ptr[j]);      // -inf once exp(x - vmax) underflows

and the equivalent tensor_Y[id] /= sum; tensor_Y[id] = log(tensor_Y[id]); for
the generic strided case.

Fix

Evaluate the numerically stable form

(x - max) - log(sum(exp(x - max)))

instead, which does not depend on exp(x - max) staying above the underflow
threshold. Both code paths of the operator are changed; the plain Softmax path is
left exactly as it was, so generated code for Softmax is unchanged.

Tests

There were no LogSoftmax tests at all, so this adds five ONNX test models plus
the matching gtest cases in TestCustomModelsFromONNX.cxx:

model shape axis purpose
LogSoftmax1d [3] -1 ordinary case, last-axis path
LogSoftmax2d [2, 3] -1 ordinary case, several rows
LogSoftmax3d [2, 3, 4] 1 ordinary case, generic strided path
LogSoftmaxLargeRange [2, 3] -1 regression: inputs 100 apart
LogSoftmaxLargeRangeAxis0 [3, 2] 0 regression: generic path, inputs 100 apart

The two large-range models cover both code paths of the operator, and both
returned -inf before this change. Their reference outputs are computed by a
new _logsoftmax_reference entry in EXPECTED_OVERRIDES, which evaluates the
stable formula in double precision — needed because onnx.reference computes
log(softmax(x)) and is wrong here. The three ordinary models keep using
onnx.reference, so they also show the fix does not change results in the normal
range.

How this was verified

I could not build ROOT locally (remote-only for this session), so the gtest suite
was not executed here. What I did run:

  1. Regression reproduced and fix confirmed with a standalone C++ harness
    (g++ -std=c++17 -O2 -Wall) that runs the code the operator emits, old and
    new form, for both the last-axis and the generic path, against a float64
    reference:

    LogSoftmaxLargeRange [2,3] axis=-1
       ref:               -200         -100            0            0         -100         -200
       old:               -inf     -99.9831            0            0     -99.9831         -inf
       new:               -200         -100            0            0         -100         -200
       max|old-ref| = inf   max|new-ref| = 0
    LogSoftmaxLargeRangeAxis0 [3,2] axis=0
       ref:               -200         -200         -100         -100            0            0
       old:               -inf         -inf     -99.9831     -99.9831            0            0
       new:               -200         -200         -100         -100            0            0
       max|old-ref| = inf   max|new-ref| = 0
    

    The plain Softmax path is unchanged and still normalizes rows to 1.0, and the
    narrow-range case is unaffected (max deviation 7.8e-08 before and after).

  2. Test data validated end to end: generate_input_models.py was run to
    produce the five models and reference files, each model passes
    onnx.checker.check_model, and every reference file was cross-checked against
    an emulation of the fixed SOFIE algorithm — max deviation 2.38e-07 against
    the test tolerance of 1e-3.

  3. ruff check (ROOT's ruff.toml) passes on the Python changes; the added C++
    lines follow the surrounding formatting of both files.

The TestCustomModelsFromONNX gtest run is left to CI.

Fixes #23547

…ximum

ROperator_Softmax computed LogSoftmax as log(softmax(x)): it normalized
exp(x - max) by the sum and then took the log of the result. When an input is
more than ~88 below the maximum, exp(x - max) underflows to 0 in float, so the
log returns -inf instead of a large negative number, and values close to the
underflow lose precision as well (e.g. -99.98 instead of -100).

Evaluate the numerically stable form (x - max) - log(sum(exp(x - max)))
instead, which does not depend on exp(x - max) staying above the underflow
threshold. Both code paths of the operator are fixed: the contiguous last-axis
case and the generic strided case. The plain Softmax path is unchanged.

There were no LogSoftmax tests, so this adds five ONNX test models
(LogSoftmax1d/2d/3d for the ordinary cases, plus LogSoftmaxLargeRange and
LogSoftmaxLargeRangeAxis0, which cover both code paths with inputs 100 apart
and returned -inf before). The reference outputs of the two large-range models
are computed with the stable formula in double precision, because onnx.reference
computes log(softmax(x)) and is wrong for the same reason.

Fixes root-project#23547
…gSoftmax fix

The formatting checks flagged the lines added by the previous commit:

- clang-format 20 wants the one-line loop bodies split and `size - 1` spaced,
  so the new generic-path block is now written the way .clang-format formats
  those lines. Only the host code is reformatted; the inference code that
  ROperator_Softmax emits is unchanged.
- ruff format, which the CI runs with --preview over the changed ranges, wants
  double quotes, so the new make_LogSoftmax* builders use them. The surrounding
  functions in generate_input_models.py still use single quotes, but they lie
  outside the changed ranges the check covers and are left untouched.

Verified with the same commands the CI runs: clang-format-20 restricted to the
diff hunks via --lines, and ruff format --diff --preview --range over the
Python hunks. Both are clean now.
@github-actions

Copy link
Copy Markdown

Test Results

    24 files      24 suites   4d 1h 10m 37s ⏱️
 3 870 tests  3 867 ✅ 0 💤 3 ❌
83 755 runs  83 751 ✅ 0 💤 4 ❌

For more details on these failures, see this check.

Results for commit c99473b.

@guitargeek

Copy link
Copy Markdown
Contributor

Duplicate of #23554, which I prefer because it added fewer lines of code and because it was created by the guy who opened the issue. Sorry!

@guitargeek guitargeek closed this Sep 30, 2026
@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Agreed — closing mine in favour of #23554.

I checked the two diffs first in case something was worth carrying over, and there is one difference: #23551 also adds LogSoftmax1d, LogSoftmax2d and LogSoftmax3d (ordinary-shape coverage for both the last-axis and generic-axis code paths, no underflow involved). #23554 has only the two LargeRange tests. If you want that basic-shape coverage it can come across; otherwise the two LargeRange tests are the ones that actually pin this bug, and I agree smaller is better for the same fix.

For the record on the red checks, since it is worth knowing before you merge either: mac27 ARM64, mac-beta ARM64, alma10 minimal build and Test Results fail on #23551, but they are not from this diff — they are failing on other contributors' PRs against the same base (#23549 fails the same four, #23546 fails a four-check set including the same three). Root's master is currently red on the same platforms. Nothing in the Softmax change touches those jobs.

No hard feelings, and thanks for taking the time to compare them rather than just closing one.

@aniruddhaadak80

Copy link
Copy Markdown
Contributor Author

Correction on my part: you had already closed it by the time I posted — your comment landed at 20:33 and I read it after writing mine. No action needed on this PR; the diff comparison and the CI-baseline note above still stand, and #23551 is closed rather than closed-by-me.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[tmva][sofie] LogSoftmax returns -inf when the inputs are far apart

3 participants