Fix scatter_reduce(reduce="mean") producing incorrect ONNX export results#2829
Fix scatter_reduce(reduce="mean") producing incorrect ONNX export results#2829justinchuby with Copilot wants to merge 5 commits into
Conversation
ONNX ScatterElements doesn't support 'mean' reduction. Previously the code mapped 'mean' to 'none' (no reduction), which just overwrote values instead of computing the mean. The fix implements mean as sum/count: - scatter_sum: ScatterElements with reduction='add' onto zeros - scatter_count: ScatterElements of ones with reduction='add' onto zeros - For include_self=True: add self to sum and 1 to count - For include_self=False: use max(count, 1) to avoid div-by-zero (positions with count=0 also have sum=0, so 0/1=0 is correct) Also removes the xfail for scatter_reduce mean in ops_test_data.py and adds e2e tests for both include_self=True and include_self=False cases. Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2829 +/- ##
==========================================
+ Coverage 71.78% 72.64% +0.86%
==========================================
Files 239 265 +26
Lines 28989 32208 +3219
Branches 2859 3041 +182
==========================================
+ Hits 20809 23398 +2589
- Misses 7209 7778 +569
- Partials 971 1032 +61 ☔ View full report in Codecov by Harness. |
|
@copilot CI errors |
Co-authored-by: justinchuby <11205048+justinchuby@users.noreply.github.com>
Fixed in 6614c55. There were two CI failures:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR fixes ONNX lowering for aten_scatter_reduce(..., reduce="mean") so exported ONNX programs match PyTorch semantics (including correct include_self=False behavior when no values are scattered into a position).
Changes:
- Implement
reduce="mean"inaten_scatter_reduceviasum/count(andWhereto preserveselfvalues wheninclude_self=Falseandcount==0). - Update
scatter_reducetest expectations (xfail) to remove the blanket"mean"unsupported marker and narrow failures to specific dtype cases. - Add e2e coverage for
scatter_reduce(mean)export for bothinclude_selfsettings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
onnxscript/function_libs/torch_lib/ops/core.py |
Adds explicit ONNX lowering for scatter_reduce(reduce="mean") using ScatterElements(reduction="add") + division, including include_self=False “preserve self when no updates” handling. |
tests/function_libs/torch_lib/ops_test_data.py |
Adjusts xfail coverage for scatter_reduce(mean) now that mean lowering exists, scoping remaining failures to specific dtype limitations. |
tests/function_libs/torch_lib/e2e_ops_tests.py |
Adds new e2e export tests targeting the reported scatter_reduce(mean) issue for both include_self=False and include_self=True. |
aten_scatter_reduceincore.pyto implementreduce="mean"correctlyinclude_self=True: sum/count approach including self in bothinclude_self=False: useWhereto preserveself[i]for positions with no scattered values (fixes CI failure where original values were incorrectly replaced with 0)e2e_ops_tests.py(forward method signature too long)xfailinops_test_data.pyto remove general mean xfailscatter_reducewithreduce="mean"for bothinclude_selfvaluesOriginal prompt
This section details on the original issue you should resolve
<issue_title>torch.export + torch.onnx.export(dynamo=True) gives incorrect results for scatter_reduce_(reduce="mean")</issue_title>
<issue_description>### 🐛 Describe the bug
Observed behaviour:
scatter_reduce(mean)shows a large mismatch after export:max_abs_diff: 10.0mean_abs_diff: 5.5sum/countcontrol matches exactly:max_abs_diff: 0.0mean_abs_diff: 0.0Expected behaviour:
scatter_reduce_(reduce="mean", include_self=False)Impact:
Environment:
torch:2.7.1onnxruntime:1.24.1python:3.12os:macOS (darwin 24.6.0)Code example:
Example output: