Skip to content

fix: Allow rank differences in aten.expand #2234

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

gs-olive
Copy link
Collaborator

Description

  • Add support for aten.expand.default in Dynamo converter registry
  • Build converter to support rank-padding for input Tensors, in line with the existing Torch behavior
  • Add test case to validate new behavior, in addition to existing cases validating old behavior

Fixes #2183

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

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

@gs-olive gs-olive added Story: ATen Op Support component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths labels Aug 16, 2023
@gs-olive gs-olive requested a review from apbose August 16, 2023 00:53
@github-actions github-actions bot added component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: tests Issues re: Tests labels Aug 16, 2023
@github-actions github-actions bot requested review from narendasan August 16, 2023 00:53
Copy link
Collaborator

@apbose apbose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As such the changes look good. But I think this was already taken care in the slice/ops.py. This condition was getting encountered in aten::where cases where the two inputs were not of equal dimensions. However since there was no aten.ops.expand in dynamo, it was picking up the acc_ops implementation. Its good that a test case for this in expand has been added.
We should either keep this or the slice one, and accordingly change the call to expand elsewhere.

@gs-olive gs-olive force-pushed the expand_converter_fix branch from 5910cf2 to 5546ae7 Compare August 17, 2023 21:57
- Add support for `aten.expand.default` in Dynamo converter registry
- Build converter to support rank-padding for input Tensors, in line
with the existing Torch behavior
- Add test case to validate new behavior, in addition to existing cases
validating old behavior
@gs-olive gs-olive force-pushed the expand_converter_fix branch from 5546ae7 to 50a9ce3 Compare August 17, 2023 22:00
@gs-olive
Copy link
Collaborator Author

Thanks for the comments and suggestions. I have merged the implementations into one expand implementation now, which is in slice/ops.

@gs-olive gs-olive requested a review from apbose August 17, 2023 22:02
Copy link
Collaborator

@narendasan narendasan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gs-olive gs-olive merged commit 56b8950 into pytorch:main Aug 22, 2023
@gs-olive gs-olive deleted the expand_converter_fix branch August 22, 2023 19:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
cla signed component: api [Python] Issues re: Python API component: conversion Issues re: Conversion stage component: converters Issues re: Specific op converters component: dynamo Issues relating to the `torch.compile` or `torch._dynamo.export` paths component: tests Issues re: Tests Story: ATen Op Support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 [Bug] aten.expand fails when rank disagrees with tensor shape
4 participants