Fix Where discarding the sign of a selected -0.0 - #32192
Open
Kevin Xu (keevin-xu) wants to merge 2 commits into
Open
Fix Where discarding the sign of a selected -0.0#32192Kevin Xu (keevin-xu) wants to merge 2 commits into
Kevin Xu (keevin-xu) wants to merge 2 commits into
Conversation
Where merges its X_selection and Y_selection with (X_selection != default value) ? X_selection : Y_selection, using "differs from the default" as a proxy for "was selected". For floats the default is +0.0 and -0.0 != 0.0 is false, so a genuinely selected negative zero was discarded and the other operand's default +0.0 used instead. A negative zero can only be present in a selection tensor if it was selected, since the default has its sign bit clear, so admit it explicitly. MSG
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
Author
|
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes CPU Where so selected negative-zero values retain their sign.
Changes:
- Adds signed-zero-aware selection detection.
- Adds regression tests for equal-shape and broadcast paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
onnxruntime/core/providers/cpu/tensor/where_op.cc |
Preserves selected negative zero during merging. |
onnxruntime/test/providers/cpu/tensor/where_op_test.cc |
Adds float signed-zero regression coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Where is selection, so a selected -0.0 must come back as -0.0. OpTester compares floating | ||
| // point outputs numerically and -0.0f == 0.0f, so a lost sign is invisible to it; check the | ||
| // sign bit directly instead. | ||
| void ExpectSignBits(const std::vector<OrtValue>& fetches, const std::vector<float>& expected) { |
The fix is templated and double is a registered Where kernel, but the tests only exercised float. Template the verifier and the four cases and instantiate each for float and double, matching how WhereBasicNumericTest and WhereBroadcastTest already cover both. On the unpatched kernel each of the three affected cases now reports two sign-bit failures, one per instantiation, confirming the double path is exercised rather than shadowed by the float call.
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
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.
Description
Replaces the two
!= T{}comparisons inWhere's merge step with aWasSelectedhelper that alsoadmits negative zero, and adds four regression tests to
WhereOpTest.Where<T>::Computebuilds anX_selectionand aY_selection, each holding the defaultT{}where the element was not selected, then merges them with
(X_selection != default value) ? X_selection : Y_selection. For floats the default is+0.0, and-0.0 == 0.0. So a-0.0selected fromXcompares equal to the default, the merge treats it asunselected, and takes
Y_selection, which holds+0.0at that position. At equal shapesY_selectionis the untested fall-through of that merge, which is why a-0.0fromYsurvivesthere but not when
Yis the broadcast scalar operand ofMergeScalarAndVector.A negative zero can only be present in a selection tensor if it was selected, since the default has
its sign bit clear, so admitting it is sufficient. Selecting
+0.0fromXstill falls through toY_selection, which holds+0.0at that position, so nothing else changes.For the CPU kernel the affected registered floating-point types are
floatanddouble.MLFloat16andBFloat16are present in the registration block but commented out, and thestd::stringspecialization uses a separate overload where the empty string is the default and isindistinguishable between the two selection arms, so it is unaffected.
OpTestercompares floating-point outputs numerically and-0.0f == 0.0f, so a lost sign isinvisible to it; the new tests check the sign bit through
SetCustomOutputVerifier, as in #31477.Three of the four fail before this change and all pass after, with
onnxruntime_provider_testgreenon macOS arm64 (5553 passed, 0 failed).
Motivation and Context
Whereis elementwise selection rather than arithmetic, so the selected element should bepreserved, and
+0.0and-0.0are distinct IEEE-754 values with distinct bit patterns.onnx.referencepreserves the negative zero in both cases; ONNX Runtime does not.Fixes #32191.