fix onnxscript export for irfft #2770
Conversation
Signed-off-by: Simon Byrne <[email protected]>
Signed-off-by: Simon Byrne <[email protected]>
simonbyrne
force-pushed
the
sbyrne/irfft
branch
from
6baaef6 to
b9462d2
Compare
| axis=dimension, | ||
| inverse=True, | ||
| onesided=False, | ||
| onesided=True, |
There was a problem hiding this comment.
Is there a way to implement this with how the dft op is defined currently? I think the current implementation has issues with normalization, but I couldn't figure out what went wrong. You expertise is appreciated!
Given the onnx update is not merged and implemented yet, we are not able to incorporate the change to torchlib at the moment as we treat it as UB (unless the current behavior is actually correct in runtimes?).
There was a problem hiding this comment.
I think the current implementation may be UB itself: from what I can tell, it treats the n//2+1 length matrix as if it was actually length n. Most of the time this means that you will be accessing unset memory and get small values (e.g. 1e-300), so it is equivalent to roughly scaling most of the factors by 1/2 (except for the 0th frequency, so the offset remains the same).
There was a problem hiding this comment.
Actually, I take that back: since it passes the fft length, it won't be UB, but it still will zero pad (and hence give the wrong answer).
There was a problem hiding this comment.
It would be possible to implement the correct behavior given what we have currently (you would need to extend the array and reverse/conjugate the values), but it would be suboptimal. If the patch to ONNX is accepted, what would be the timeline to getting it updated here?
There was a problem hiding this comment.
When the patch is accepted, depending on consensus it may or may not need to be in a new opset. If it is in the new opset, we need to update pytorch code. It will be accepted here if it is just a clarification of the old opset.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def Pad( | ||
| self, | ||
| data: T_Pad, | ||
| pads: INT64, | ||
| constant_value: Optional[T_Pad] = None, | ||
| axes: Optional[Tind_Pad] = None, | ||
| *, | ||
| mode: str = "constant", | ||
| ) -> T_Pad: |
Check warning
Code scanning / CodeQL
Signature mismatch in overriding method Warning
| UINT8, | ||
| ) | ||
|
|
||
| def Reshape(self, data: T_Reshape, shape: INT64, *, allowzero: int = 0) -> T_Reshape: |
Check warning
Code scanning / CodeQL
Signature mismatch in overriding method Warning
| UINT8, | ||
| ) | ||
|
|
||
| def Unsqueeze(self, data: T_Unsqueeze, axes: INT64) -> T_Unsqueeze: |
Check warning
Code scanning / CodeQL
Signature mismatch in overriding method Warning
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2770 +/- ##
===========================================
- Coverage 70.45% 11.72% -58.73%
===========================================
Files 228 219 -9
Lines 27252 21906 -5346
Branches 2759 2227 -532
===========================================
- Hits 19201 2569 -16632
- Misses 7099 19328 +12229
+ Partials 952 9 -943 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
2 participants
This fixes the onnxscript export for the irfft function.
Fixes onnx/onnx#5920, and adds support to the changes in onnx/onnx#7574 and microsoft/onnxruntime#27028.
Most of the diff is due to the onnx_opset generated code changes from onnx/onnx#5920. That can be removed if you would prefer.