Propagate NaNs in the CPU min and max operators #21492
Conversation
|
Should there be a test where the NaN is the scalar to check it is propagated throughout the broadcast? e.g. input {2,2} with no NaN and {1} with a NaN should result in all NaN in the output IIUC |
|
@microsoft-github-policy-service agree company="G-Research" |
Good idea thanks, I've added those tests now and fixed the formatting errors. |
|
/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline |
|
/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models,Linux Android Emulator QNN CI Pipeline |
|
/azp run Android CI Pipeline,iOS CI Pipeline,ONNX Runtime React Native CI Pipeline |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
Azure Pipelines successfully started running 10 pipeline(s). |
|
It looks like there are some issues with the asan runs and the MLFloat16 tests:
And the web builds also seem to have a problem with these new tests: https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=1447211&view=logs&j=990616d7-1f75-5fe5-5c67-c84a39482fba&t=67a495a8-8444-507d-6780-4eb1ddef3707&l=14255 The same problem was run into in #19984 (comment), where there weren't any changes to I'm not sure what could be causing this, it seems like a bug in Eigen rather than onnxruntime at first glance. The best way forward is probably to do the same as before and revert the MLFloat16 related changes for now, and I can make a new issue to follow up on this. |
|
/azp run Windows ARM64 QNN CI Pipeline,Windows x64 QNN CI Pipeline,Windows CPU CI Pipeline,Windows GPU CI Pipeline,Windows GPU TensorRT CI Pipeline,ONNX Runtime Web CI Pipeline,Linux CPU CI Pipeline,Linux CPU Minimal Build E2E CI Pipeline,Linux GPU CI Pipeline,Linux GPU TensorRT CI Pipeline |
|
/azp run Linux OpenVINO CI Pipeline,Linux QNN CI Pipeline,MacOS CI Pipeline,orttraining-amd-gpu-ci-pipeline,orttraining-linux-ci-pipeline,orttraining-linux-gpu-ci-pipeline,orttraining-ortmodule-distributed,onnxruntime-binary-size-checks-ci-pipeline,Big Models,Linux Android Emulator QNN CI Pipeline |
|
/azp run Android CI Pipeline,iOS CI Pipeline,ONNX Runtime React Native CI Pipeline |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
Azure Pipelines successfully started running 10 pipeline(s). |
This makes min and max with NaN for either operand always return NaN for float16 data, matching the behaviour of float and double. The behaviour for floats and doubles was previously fixed for the CPU provider in #21492 and the CUDA provider in #19984, but these PRs didn't fix the behaviour for float16 due to tests causing asan errors. The memory access violations with float16 data have now been fixed in #22135, so this PR is a follow up to make float16 min and max behave the same as float and double for both the CPU and CUDA providers now that we can add tests for this. ### Motivation and Context Relevant previous issues (not float16 specific): * #21455 * onnx/onnx#6003
Description
Propagates NaN values in the min and max operators so that min or max with a NaN in either input always produces NaN.
Only fixes NaN propagation for float and double data types due to invalid read errors when testing NaNs with float16 data: #21492 (comment).
Motivation and Context
Fixes #21455