qonnx icon indicating copy to clipboard operation
qonnx copied to clipboard

Add cleanup transformation sorting inputs of commutative operations

Open iksnagreb opened this issue 2 years ago • 2 comments

Adds the cleanup transformation requested in #84

Closes #84

iksnagreb avatar Nov 14 '23 13:11 iksnagreb

Hi Christoph,

Thanks for the PR. I agree that this would be a useful cleanup transformation, but one remark after a brief review: since this gets called as part of the ModelWrapper.cleanup() which essentially gets called after every single transformation, and since the ONNX export behavior we observe from e.g. PyTorch has changed over the course of versions, we should try to add some extra checks for future-proofing here. Could you provide a unit test that exercises this transformation? Might be easiest to take one of the existing ONNX testcases and artificially manipulate the input list as part of the testcase such that the initialized input appears first.

maltanar avatar Feb 23 '24 13:02 maltanar

Could you provide a unit test that exercises this transformation?

Has been added now, testing this transformation on the Sum operator and some input/initializer combinations.

iksnagreb avatar Apr 25 '24 14:04 iksnagreb

LGTM, ready to merge. Thanks @iksnagreb !

maltanar avatar Sep 12 '24 07:09 maltanar