Add contiguous_strides as a correct replacement of defaultStride #67789
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`at::defaultStride` was added in #18779 As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. [ghstack-poisoned]
CI Flow Status⚛️ CI FlowRuleset - Version:
You can add a comment to the PR and tag @pytorchbot with the following commands: # ciflow rerun, "ciflow/default" will always be added automatically
@pytorchbot ciflow rerun
# ciflow rerun with additional labels "-l <ciflow/label_name>", which is equivalent to adding these labels manually and trigger the rerun
@pytorchbot ciflow rerun -l ciflow/scheduled -l ciflow/slowFor more information, please take a look at the CI Flow Wiki. |
Closed
🔗 Helpful links
💊 CI failures summary and remediationsAs of commit 65b9aea (more details on the Dr. CI page):
🕵️ 1 new failure recognized by patternsThe following CI failures do not appear to be due to upstream breakages:
|
|
@IvanYashchuk This correct the error we discussed for matrices with one of its sizes zero being zero. It also simplifies the writing of structured kernesl with fortran backends. |
…Stride" `at::defaultStride` was added in #18779 As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779 As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
IvanYashchuk
reviewed
Nov 4, 2021
| auto solution_strides = at::detail::defaultStrides(self_broadcast_size); | ||
| solution_strides[ndim - 2] = 1; | ||
| solution_strides[ndim - 1] = nrows; | ||
| const auto solution_strides = at::native::contiguous_strides(self_broadcast_size, /*f-contig=*/true); |
There was a problem hiding this comment.
Thanks for fixing that!
Why the tests were not failing? You mentioned that autograd doesn't like matrices with 0 strides and sample_inputs_linalg_solve from common_methods_invocations.py has 0x0 input matrix case.
There was a problem hiding this comment.
It's forward AD that was failing. Can it be the case that this function does not support forward AD?
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
This was referenced Nov 11, 2021
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
mruberry
approved these changes
Nov 12, 2021
There was a problem hiding this comment.
Cool!
Let's give @IvanYashchuk and @nikitaved a chance to review, too.
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-p F438 oisoned]
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Maybe we should build with these same flags, @dagitses @malfet, since they block landing? |
|
I just fixed this one @mruberry. But yes, it'd be great to, at least, add this one flag to the open CI. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
…Stride" `at::defaultStride` was added in #18779. As it was noted in that PR, it differs from the actual computation of the default strides when one or more of the dimensions of the tensor are zero. See #18779 (comment) We add two functions, `contiguous_strides` and `contiguous_strides_vec` which correct this issue and we replace the previous (wrong) uses of `defaultStride`. cc jianyuh nikitaved pearu mruberry walterddr IvanYashchuk xwang233 Lezcano Differential Revision: [D32684852](https://our.internmc.facebook.com/intern/diff/D32684852) [ghstack-poisoned]
|
@mruberry has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack:
at::defaultStridewas added in #18779.As it was noted in that PR, it differs from the actual computation of
the default strides when one or more of the dimensions of the tensor are
zero. See #18779 (comment)
We add two functions,
contiguous_stridesandcontiguous_strides_vecwhich correct this issue and we replace the previous (wrong) uses of
defaultStride.cc @jianyuh @nikitaved @pearu @mruberry @walterddr @IvanYashchuk @xwang233 @lezcano
Differential Revision: D32684852