[ONNX] Fix dtype for NonMaxSuppression #7056

justinchuby · 2022-12-31T16:09:55Z

Explicitly cast inputs to NonMaxSuppression to float32 to accommodate float64 inputs because NonMaxSuppression only supports float32 coordinates. This is necessary to unblock pytorch/pytorch#78442 and pytorch/pytorch#86146.

Also removed the use of deprecated _cast_Long.

cc @neginraoof @BowenBao @mruberry

…ch eager execution " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…on " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

BowenBao · 2023-01-01T19:58:56Z

Explicitly cast inputs to NonMaxSuppression to float32 to accommodate float64 inputs because NonMaxSuppression only supports float32 coordinates.

Should we do this in scalar_type_analysis pass, wdyt?

justinchuby · 2023-01-03T03:55:52Z

That's a good point. I will see how it can be done

…ch eager execution " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…on " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

justinchuby · 2023-02-13T17:53:51Z

@BowenBao I think this change is a much more efficient one. Can we resurrect this change?

BowenBao · 2023-02-13T17:59:51Z

@datumbox please take a look.

justinchuby · 2023-02-14T16:36:53Z

@datumbox @pmeier
this is needed for the pytorch 2.0 release. Could you help getting the reviews needed? Thanks a lot!

datumbox · 2023-02-14T17:34:13Z

@justinchuby I'm no longer in the maintainer group to offer a stamp and help you merge.

Not sure if any of the remaining folks from EU are online now. Perhaps @malfet @atalman could stamp?

datumbox · 2023-02-14T17:35:44Z

Also it seems that one of the tests is failing? Could be related.

malfet · 2023-02-14T17:56:04Z

@datumbox I can send you an invite, if you need one.

NicolasHug · 2023-02-14T18:15:41Z

Thanks for the PR @justinchuby

Moved symbolic functions to the module scope and cleaned up imports for symplicity.

Any chance we could revert this change? It makes the diff a little difficult to review

pmeier · 2023-02-14T19:15:59Z

Thanks for the PR @justinchuby. Could you clarify what review you need from us? Happy to help get this merged before our branch cut on Feb 17. As @datumbox mentioned, we have a (previously undetected) ONNX failure: #7247.

@NicolasHug I think we need to prioritize getting this landed and CI green over potential style nits given that our branch cut is looming ahead. Happy to revisit afterwards.

justinchuby · 2023-02-14T19:17:49Z

Thanks for the PR @justinchuby. Could you clarify what review you need from us? Happy to help get this merged before our branch cut on Feb 17. As @datumbox mentioned, we have a (previously undetected) ONNX failure: #7247.

@NicolasHug I think we need to prioritize getting this landed and CI green over potential style nits given that our branch cut is looming ahead. Happy to revisit afterwards.

thanks! I will work on this today. We need the "Fix dtype for NonMaxSuppression" fix to allow pytorch/pytorch#86146 to land because otherwise a vision test would fail.

justinchuby · 2023-02-15T00:45:39Z

@pmeier @NicolasHug I changed this PR to be minimal and found the cause for the test failure. We need pytorch/pytorch#94870 then this then pytorch/pytorch#86146, in this order. Thanks a lot!

The assert statement blocks tensors with unknown ranks. This change unblocks those cases. Needed for pytorch/vision#7056 Verified against pytorch/vision#7056 Pull Request resolved: #94870 Approved by: https://github.com/BowenBao

pmeier · 2023-02-15T06:57:34Z

Since pytorch/pytorch#94870 is already merged, we should have access to it with todays nightly release in roughly a couple of hours. Will merge then if CI is green. Just so I understand correctly, #7249 is not strictly needed but improves code quality, right? Meaning, it would be good to merge soon, but can wait until after the branch cut?

pmeier · 2023-02-15T10:39:51Z

ONNX test is green: https://app.circleci.com/pipelines/github/pytorch/vision/23230/workflows/c3c3853e-6c98-4571-aae7-760bb6c36967/jobs/1829412 🎉

github-actions · 2023-02-15T10:42:49Z

Hey @pmeier!

You merged this PR, but no labels were added. The list of valid labels is available at https://github.com/pytorch/vision/blob/main/.github/process_commit.py

justinchuby · 2023-02-15T15:35:33Z

Just so I understand correctly, #7249 is not strictly needed but improves code quality, right? Meaning, it would be good to merge soon, but can wait until after the branch cut?

That's accurate

…ch eager execution " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…on " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…ch eager execution " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…on " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…ch eager execution " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

…on " - Fixes #78442 - Change floor_divide to round down to match eager. The old behavior for floor_divide (which was actually trunc divide) is deprecated and removed in eager mode. (See #78442) - Remove division logic in opset 10 because it is duplicated from opset 9. - Add tests to `remainder`, `div`, `floor_divide` and `true_divide` ### Blocked by - pytorch/vision#7056 [ghstack-poisoned]

Summary: Reviewed By: vmoens Differential Revision: D44416637 fbshipit-source-id: c06807ab7d9d71f2272761bf366411268ed5b462 Co-authored-by: Nikita Shulga <nshulga@fb.com> Co-authored-by: Philip Meier <github.pmeier@posteo.de>

[ONNX] Fix dtype for NonMaxSuppression; misc improvements

4949b5a

facebook-github-bot added the cla signed label Dec 31, 2022

justinchuby marked this pull request as ready for review December 31, 2022 16:14

Sort imports

c088d46

justinchuby mentioned this pull request Dec 31, 2022

[ONNX] Update floor_divide behavior to match eager execution pytorch/pytorch#86146

Closed

tests

0a08a3c

Merge branch 'main' into justinchuby/onnx

0a63f3d

BowenBao approved these changes Feb 13, 2023

View reviewed changes

justinchuby mentioned this pull request Feb 14, 2023

TODOs before 0.15 release #7217

Closed

49 tasks

Merge branch 'main' into justinchuby/onnx

45c8952

Merge branch 'main' into justinchuby/onnx

e16575b

justinchuby and others added 3 commits February 14, 2023 15:44

Merge branch 'pytorch:main' into justinchuby/onnx

5f5759a

Revert misc changes

a1b231a

format

8e5a546

justinchuby mentioned this pull request Feb 15, 2023

[ONNX] Fix assert in cat pytorch/pytorch#94870

Closed

justinchuby changed the title ~~[ONNX] Fix dtype for NonMaxSuppression; misc improvements~~ [ONNX] Fix dtype for NonMaxSuppression Feb 15, 2023

Merge branch 'main' into justinchuby/onnx

8ae021b

pmeier merged commit f627b9d into pytorch:main Feb 15, 2023

pmeier added the module: onnx label Feb 15, 2023

justinchuby deleted the justinchuby/onnx branch February 15, 2023 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ONNX] Fix dtype for NonMaxSuppression #7056

[ONNX] Fix dtype for NonMaxSuppression #7056

justinchuby commented Dec 31, 2022 •

edited by pytorch-bot bot

BowenBao commented Jan 1, 2023

justinchuby commented Jan 3, 2023

justinchuby commented Feb 13, 2023

BowenBao commented Feb 13, 2023

justinchuby commented Feb 14, 2023 •

edited

datumbox commented Feb 14, 2023

datumbox commented Feb 14, 2023

malfet commented Feb 14, 2023

NicolasHug commented Feb 14, 2023

pmeier commented Feb 14, 2023

justinchuby commented Feb 14, 2023 •

edited

justinchuby commented Feb 15, 2023

pmeier commented Feb 15, 2023

pmeier commented Feb 15, 2023 •

edited

github-actions bot commented Feb 15, 2023

justinchuby commented Feb 15, 2023

[ONNX] Fix dtype for NonMaxSuppression #7056

[ONNX] Fix dtype for NonMaxSuppression #7056

Conversation

justinchuby commented Dec 31, 2022 • edited by pytorch-bot bot

BowenBao commented Jan 1, 2023

justinchuby commented Jan 3, 2023

justinchuby commented Feb 13, 2023

BowenBao commented Feb 13, 2023

justinchuby commented Feb 14, 2023 • edited

datumbox commented Feb 14, 2023

datumbox commented Feb 14, 2023

malfet commented Feb 14, 2023

NicolasHug commented Feb 14, 2023

pmeier commented Feb 14, 2023

justinchuby commented Feb 14, 2023 • edited

justinchuby commented Feb 15, 2023

pmeier commented Feb 15, 2023

pmeier commented Feb 15, 2023 • edited

github-actions bot commented Feb 15, 2023

justinchuby commented Feb 15, 2023

justinchuby commented Dec 31, 2022 •

edited by pytorch-bot bot

justinchuby commented Feb 14, 2023 •

edited

justinchuby commented Feb 14, 2023 •

edited

pmeier commented Feb 15, 2023 •

edited