Update compute elementwise output strides semantics#164252
Update compute elementwise output strides semantics#164252morrison-turnansky wants to merge 6 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/164252
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 57f8206 with merge base 8d53968 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@Lucaskabela I thought it would be cleaner to start on new PR. The above fixes the case when one tensor is given. The reason this works is that As an alternative fix to avoid this extra cost, there is a python implementation of the above function, namely |
|
@pytorchbot label "topic: not user facing" |
| # If not, eager clone creates contiguous strides | ||
| computed_stride = None | ||
| if utils.is_non_overlapping_and_dense(input): | ||
| computed_stride = input.stride() |
There was a problem hiding this comment.
Seems removing this line is causing the unit test for this bug to fail :/
There was a problem hiding this comment.
@Lucaskabela which failing test are you referring to?
The only failing test I saw in the CI was
test/test_dynamic_shapes.py: test_prims_non_overlapping_and_dense.
I had missed renaming one of the function calls. Updated and pushed change.
Lucaskabela
left a comment
There was a problem hiding this comment.
Seems this change still has some error to work through
|
I took a look at this function - I think we can refactor it out to leverage it here |
d473557 to
17f9b97
Compare
2eaf53d to
7acf73c
Compare
|
@Lucaskabela Circling back on this. I had fixed the failing test in the CI pass. If there are other changes that you want can you elaborate? Unclear what the intent of this comment was. which function?
|
|
Hey @morrison-turnansky I don't recall what i was referring to here unfortunately - going to cc @zou3519 here since I will be on PTO next two weeks |
Lucaskabela
left a comment
There was a problem hiding this comment.
Thanks for the changes! I think we should add test cases that this change is covering our previous change is not, otherwise LGTM
| @@ -897,9 +897,9 @@ def test_non_overlapping_and_dense_unbacked(self): | |||
| ) | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
I think @zou3519 had some extra test cases for us to consider here; if not, I still think it would be worth adding an additional test case or two to make sure we are doing the right thing here
There was a problem hiding this comment.
@Lucaskabela Following up. Any addtional action?
|
@Lucaskabela Enjoy the PTO! |
… stride of meta tensor in 1 tensor case
…s_non_overlapping_and_dense_or_false
…s to use semeantics inherited from empty_like
…ims_is_non_overlapping_and_dense_or_false
7acf73c to
57f8206
Compare
|
CI is passing. Please let me know if you want any additional changes, and/or resolve requested changes if you found my updates were sufficient. Thank you. Trying to tie this up before I go on PTO for end of year holidays. |
Lucaskabela
left a comment
There was a problem hiding this comment.
Thanks for the change, I think this is generally good now :)
|
@pytorchbot merge |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Continuation of work from pytorch#161400 and pytorch#163017. Updating stride semantics for ```clone_meta``` and underlying function, ```compute_elementwise_output_strides```. Pull Request resolved: pytorch#164252 Approved by: https://github.com/Lucaskabela
Continuation of work from #161400 and #163017.
Updating stride semantics for
clone_metaand underlying function,compute_elementwise_output_strides.cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben @jataylo @chenyang78