feat: support argmin aten converter #2501

bowang007 · 2023-11-29T06:27:27Z

Description

Support argmin converter.
#2498

Checklist:

My code follows the style guidelines of this project (You can use the linters)
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas and hacks
I have made corresponding changes to the documentation
I have added tests to verify my fix or my feature
New and existing unit tests pass locally with my changes
I have added the relevant labels to my PR in so that relevant reviewers are notified

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

bowang007 · 2023-11-29T06:30:25Z

Looks like we can deduplicate the code with ArgMax converter, we can also just leave it as for now since the duplication is trivial here.

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

gs-olive

Added a few minor naming comments, otherwise looks good to me!

tests/py/dynamo/conversion/test_argmin_aten.py

zewenli98

Consider combining argmax.py and argmin.py files (maybe could be moved to reduce.py?), and doing changes for their tests that @gs-olive mentioned above. Others LGTM!

apbose

The changes look good, but as Evan said, can argmin.py and argmax.py be merged? Also you could restructure it in such a way that it has a base.py in reduce folder exposing the topK layer in a function and argmin and argmax could use that. Else the code is redundant apart from the usage of the topK layer.

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

gs-olive

Overall looks good - the new folder argmaxmin just needs to be added to setup.py to reflect in the build

zewenli98 · 2023-12-05T19:36:25Z

@bowang007 As we discussed offline, since argmin and argmax use ITopKLayer, for generality, we can have a file impl/topk.py and move related converters into this file.

github-actions

Code conforms to C++ style guidelines

github-actions

Code conforms to Python style guidelines

bowang007 · 2023-12-06T00:22:50Z

Overall looks good - the new folder argmaxmin just needs to be added to setup.py to reflect in the build

Thanks @gs-olive !
I refactored that folder into a single file topk.py.
Btw, I'm not so familiar with python packages but does this mean every time we create a folder we always need to build the package for that folder right?
Thanks!

bowang007 · 2023-12-06T00:23:51Z

@bowang007 As we discussed offline, since argmin and argmax use ITopKLayer, for generality, we can have a file impl/topk.py and move related converters into this file.

@zewenli98 Thanks!
I refactored that part, should be good to go now.
Please take a look so we can merge it by EOD.

zewenli98

Looks good to me!

gs-olive

Looks good, pending CI

github-actions

Code conforms to Python style guidelines

github-actions

Code conforms to C++ style guidelines

facebook-github-bot added the cla signed label Nov 29, 2023

github-actions bot requested a review from peri044 November 29, 2023 06:27

github-actions bot approved these changes Nov 29, 2023

View reviewed changes

bowang007 requested review from zewenli98, gs-olive, narendasan and apbose November 29, 2023 06:30

bowang007 self-assigned this Nov 29, 2023

github-actions bot approved these changes Nov 29, 2023

View reviewed changes

gs-olive reviewed Nov 29, 2023

View reviewed changes

tests/py/dynamo/conversion/test_argmin_aten.py Outdated Show resolved Hide resolved

tests/py/dynamo/conversion/test_argmin_aten.py Outdated Show resolved Hide resolved

zewenli98 reviewed Nov 29, 2023

View reviewed changes

apbose reviewed Dec 1, 2023

View reviewed changes

bowang007 requested review from zewenli98, gs-olive and apbose December 4, 2023 23:41

github-actions bot approved these changes Dec 4, 2023

View reviewed changes

bowang007 mentioned this pull request Dec 4, 2023

aten.topk #2499

Closed

github-actions bot approved these changes Dec 4, 2023

View reviewed changes

bowang007 mentioned this pull request Dec 4, 2023

Expose ITopKLayer in dynamo.conversion.impl #2216

Closed

github-actions bot approved these changes Dec 5, 2023

View reviewed changes

gs-olive reviewed Dec 5, 2023

View reviewed changes

bowang007 force-pushed the argmin_dynamo_converter branch from f2e0859 to 1f9daf5 Compare December 6, 2023 00:19

github-actions bot approved these changes Dec 6, 2023

View reviewed changes

zewenli98 approved these changes Dec 6, 2023

View reviewed changes

gs-olive approved these changes Dec 6, 2023

View reviewed changes

feat: support argmin aten converter and refactor argmax

5ec8a2b

bowang007 force-pushed the argmin_dynamo_converter branch from 1f9daf5 to 5ec8a2b Compare December 8, 2023 23:28

github-actions bot approved these changes Dec 8, 2023

View reviewed changes

bowang007 merged commit fb2d936 into main Dec 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support argmin aten converter #2501

feat: support argmin aten converter #2501

bowang007 commented Nov 29, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

bowang007 commented Nov 29, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

gs-olive left a comment

zewenli98 left a comment •

edited

Loading

apbose left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

gs-olive left a comment

zewenli98 commented Dec 5, 2023

github-actions bot left a comment

github-actions bot left a comment

bowang007 commented Dec 6, 2023

bowang007 commented Dec 6, 2023

zewenli98 left a comment

gs-olive left a comment

github-actions bot left a comment

github-actions bot left a comment

feat: support argmin aten converter #2501

feat: support argmin aten converter #2501

Conversation

bowang007 commented Nov 29, 2023 • edited Loading

Description

Checklist:

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

bowang007 commented Nov 29, 2023 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

gs-olive left a comment

Choose a reason for hiding this comment

zewenli98 left a comment • edited Loading

Choose a reason for hiding this comment

apbose left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

gs-olive left a comment

Choose a reason for hiding this comment

zewenli98 commented Dec 5, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

bowang007 commented Dec 6, 2023

bowang007 commented Dec 6, 2023

zewenli98 left a comment

Choose a reason for hiding this comment

gs-olive left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

bowang007 commented Nov 29, 2023 •

edited

Loading

bowang007 commented Nov 29, 2023 •

edited

Loading

zewenli98 left a comment •

edited

Loading