Fix: Stable implementation for ReduceLogSumExp (ONNX frontend) #32913
Conversation
There was a problem hiding this comment.
Hey, thanks!
We also looked into this in the meantime (our working version is here master...micropsi-industries:openvino:fix_logsumexp )
We were wondering about one or two points, see comments below
| const ov::Output<ov::Node> sum_node = | ||
| make_ov_reduction_op<v1::ReduceSum>(node, node.get_ov_inputs().at(0), supported_types_v2); | ||
| return {std::make_shared<v0::Log>(sum_node)}; | ||
| } |
There was a problem hiding this comment.
Thanks for the review!
The removal of reduce_log_sum was not intentional the change in this PR is meant only to introduce a stable implementation for ReduceLogSumExp.
I'll restore the original reduce_log_sum translator and its registration in the next commit.
Thanks for pointing it out!
| return {make_ov_reduction_op<v1::ReduceMin>(node, node.get_ov_inputs().at(0), supported_types_v3, false)}; | ||
| } | ||
|
|
||
| ov::OutputVector reduce_log_sum(const ov::frontend::onnx::Node& node) { |
There was a problem hiding this comment.
Thanks for pointing this out again!
The deletion of reduce_log_sum was not intentional I accidentally removed it while restructuring the file during the ReduceLogSumExp update.
I'll restore the original reduce_log_sum implementation and its registration in the next commit.
Thanks for catching this!
| auto keepdims = static_cast<bool>(node.get_attribute_value<std::int64_t>("keepdims", 1)); | ||
|
|
||
| auto reduction_axes = | ||
| (node.get_ov_inputs().size() > 1) ? get_reduction_axes_from_input(node) : get_reduction_axes_from_attr(node); |
There was a problem hiding this comment.
Q: as far as we understand, reduction axes are always given by attr in opset <= 12 and always by input starting from opset 13 -- since these are separate implementations per opset, isn't reducution_axes = get_reduction_axes_from_attr(node) enough here in the opset 1 section?
There was a problem hiding this comment.
Good question thank you!
You're right that for ONNX opset ≤ 12, axes are normally provided via the axes attribute, and input-based axes become standard only from opset 13 onward.
However, some models (and converters) still emit opset-1 with axes passed as a second input rather than an attribute. OpenVINO’s legacy opset-1 translator handled both cases, so I kept the same logic to avoid breaking existing models.
That said, I can simplify it to attribute-only if we want strict adherence to the ONNX specification.
Please let me know if you prefer simplifying it I can update the PR accordingly.
There was a problem hiding this comment.
Ah interesting point. That's beyond my expertise, so I don't have an opinion. Let's wait for the maintainer review
3 participants
Summary #32839
This patch fixes numerical instability in the ONNX frontend’s
ReduceLogSumExpoperator.The previous implementation used a naïve
log(sum(exp(x)))formulation, which overflowed toInfforvalues near
log(MAX_FLOAT32)(≈ 88.72). This caused incorrect results in the OpenVINO EP of ONNX Runtime,especially for float16 and float32 models.
Fix
Implemented a numerically stable LogSumExp computation:
This matches the behavior of:
log.cpp)The new implementation is applied to:
opset_1::reduce_log_sum_exp)opset_13::reduce_log_sum_exp)opset_18::reduce_log_sum_exp)Motivation
Fixes incorrect
Infoutputs for values ≥ 88.7 when using OpenVINOExecutionProvider.The issue was originally reported here: [link to GitHub issue].
Validation
Tested using provided reproduction script from the issue:
Notes
This fix aligns ONNX frontend behavior with the already-correct PyTorch frontend implementation.