[SOFIE] Fix ELU alpha parentheses in Generate()#21543
[SOFIE] Fix ELU alpha parentheses in Generate()#21543Neeraj-x0 wants to merge 3 commits intoroot-project:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes incorrect ELU code generation in SOFIE where the negative branch for non-default alpha was emitted without parentheses, changing the math due to operator precedence (fixes #21539).
Changes:
- Correct ELU negative-branch codegen to emit
alpha * (exp(x) - 1)instead of(alpha * exp(x)) - 1. - Minor formatting/whitespace adjustments in
ROperator_Elu.hxx.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| out << SP << SP << "tensor_" << fNY << "[id] = ((tensor_" << fNX << "[id] >= 0 )? tensor_" << fNX | ||
| << "[id] : " << OpName << "_alpha * (std::exp(tensor_" << fNX << "[id]) - 1));\n"; |
There was a problem hiding this comment.
Test coverage: the existing SOFIE ONNX ELU test appears to cover only the default alpha=1.0, which wouldn’t have caught this precedence bug. Consider adding a regression test/model with a non-default alpha (e.g. 0.5 or 2.0) so future changes to ELU codegen are validated for alpha != 1.0.
| out << "\n//------ ELU \n"; | ||
| out << SP << "for (int id = 0; id < " << length << " ; id++){\n"; | ||
| out << SP << SP << "tensor_" << fNY << "[id] = ((tensor_" << fNX << "[id] >= 0 )? tensor_" << fNX << "[id] : "<< OpName << "_alpha * std::exp(tensor_"<< fNX<<"[id]) - 1);\n"; | ||
| out << SP << SP << "tensor_" << fNY << "[id] = ((tensor_" << fNX << "[id] >= 0 )? tensor_" << fNX | ||
| << "[id] : " << OpName << "_alpha * (std::exp(tensor_" << fNX << "[id]) - 1));\n"; |
There was a problem hiding this comment.
Generate() emits generated C++ that calls std::exp(...), but ROperator_Elu doesn’t currently advertise the need for <cmath> (e.g., via GetStdLibs() like Sigmoid/Selu/Erf do, or by calling model.AddNeededStdLib("cmath") in Initialize). This can make generated model headers fail to compile for ELU-only models when <cmath> isn’t otherwise included upstream.
|
Added a regression test with |
|
Hi, thanks for the PR! It would be easier to review changes if we had one commit with the changes and one with only the formatting |
Neeraj-x0
force-pushed
the
fix/elu-alpha-parentheses
branch
3 times, most recently
from
347a45a to
5ff7408
Compare
|
@siliataider Updated Please check now |
Without parentheses, 'alpha * std::exp(x) - 1' evaluates as '(alpha * exp(x)) - 1' due to operator precedence, which differs from the correct ELU formula 'alpha * (exp(x) - 1)' when alpha != 1. Fixes root-project#21539
Adds EluAlpha.onnx (alpha=0.5) and a corresponding test case to prevent regressions for non-default alpha values in ELU codegen. Refs root-project#21539
Neeraj-x0
force-pushed
the
fix/elu-alpha-parentheses
branch
from
5ff7408 to
19dd22e
Compare
|
Running the builds now |
Test Results 22 files 22 suites 3d 4h 36m 10s ⏱️ For more details on these failures, see this check. Results for commit 19dd22e. |
The Generate() function emitted
alpha * std::exp(x) - 1which due tooperator precedence evaluates as
(alpha * exp(x)) - 1, not the correctELU formula
alpha * (exp(x) - 1). Only affects alpha != 1.0.Fixes #21539
This Pull Request:
Changes or fixes:
Fixed incorrect parentheses in
ROperator_Elu::Generate()intmva/sofie/inc/TMVA/ROperator_Elu.hxx. The negative branch of the ELUoperator now correctly emits
alpha * (std::exp(x) - 1)instead ofalpha * std::exp(x) - 1.Checklist:
This PR fixes #21539