Materialize the DFT axis default when converting opset 19 to 20 - #3023
Conversation
Opset 19 defines DFT axis as an attribute defaulting to 1. Opset 20 moved it to an input defaulting to -2. dft_19_20 read the attribute with a None default and returned early when it was absent, so the node was left untouched while the model opset was bumped, silently retargeting the transform from axis 1 to axis -2 for any input of rank greater than 3. The None guard is kept because _get_int_attribute also returns None for an attribute of unexpected type.
Justin Chu (justinchuby)
left a comment
There was a problem hiding this comment.
Thanks!
|
Mohammed Alkindi (@MohammedAlkindi) could you please fix lint |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3023 +/- ##
=======================================
Coverage 72.64% 72.65%
=======================================
Files 265 265
Lines 32251 32260 +9
Branches 3050 3050
=======================================
+ Hits 23429 23438 +9
Misses 7786 7786
Partials 1036 1036 ☔ View full report in Codecov by Harness. |
|
Lint is fixed and pushed as a33e8bf. It was one duplicate blank line in On the red that remains: the three |
There was a problem hiding this comment.
Pull request overview
This pull request fixes a semantic mismatch in the opset 19 → 20 version conversion for DFT by materializing opset-19’s default axis=1 as an explicit opset-20 axis input, preventing silent behavior changes when the axis attribute is omitted.
Changes:
- Update
dft_19_20to treat a missingaxisattribute as1(while still returningNonefor invalid/wrongly-typed attributes). - Always emit an explicit
axisConstant input when conversion applies. - Add a regression test that omits the
axisattribute and validates the converted node has a third input with value1.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| onnxscript/version_converter/_version_converter.py | Materializes opset-19 DFT.axis default (1) as an opset-20 axis input Constant to preserve semantics during conversion. |
| onnxscript/version_converter/_version_converter_test.py | Adds regression coverage for converting DFT without an axis attribute and asserting the axis input is explicitly set to 1. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The It passed on I do not have rights to re-run the job, so it needs a nudge from your side. The three |
c927620
into
microsoft:main
Opset 19 defines DFT
axisas an attribute defaulting to 1. Opset 20 moved it to an input defaulting to -2.dft_19_20read the attribute with aNonedefault and returned early when it was absent, so the node was left untouched while the model opset was bumped. For any input of rank greater than 3 the converted model silently computes a different transform.Measured on a rank-4 input with no
axisattribute, numpy as ground truth, at the parent commit:onnx 1.22.0's own converter emits an axis Constant of 1 for this model, so the fix makes onnxscript agree with it.
The existing DFT tests all set
axisexplicitly, so the default path was never exercised. The added test omits it and fails on the unpatched source with1 != 3.version_converter,optimizerandrewriter: 573 passed, 2 skipped, 6 xfailed.The
Noneguard stays:_get_int_attributealso returnsNonefor a wrongly-typed attribute. Open PR #2941 changes only the@registerdecorator on this function, so the two do not overlap.