-
Notifications
You must be signed in to change notification settings - Fork 605
Make is_contiguous check common #3083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
/// such that the strides are in non-increasing order, and the stride at position | ||
/// `k` is equal to the product of the shapes of all dimensions greater than `k`. | ||
/// Axes with a shape of 1 are ignored. | ||
pub fn is_contiguous(shape: &[usize], strides: &[usize]) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think the check could be simplified to
pub fn is_contiguous(shape: &[usize], strides: &[usize]) -> bool {
if shape.is_empty() {
return true;
}
let mut expected_stride = 1;
for (&shape, &stride) in shape.iter().zip(strides).rev() {
if shape == 1 {
continue;
}
if stride != expected_stride {
return false;
}
expected_stride *= shape;
}
true
}
But leaving this here as a note and not a required change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that strategy actually, we could even create a function that could be used to create the strides AND be used for the checks:
pub fn contiguous_strides(shape: &[usize]) -> impl Iterator<Item = usize> {
let mut current = 1;
shape
.iter()
.rev()
.map(|val| {
current *= val;
current
})
.rev()
}
pub fn is_contiguous(shape: &[usize], strides: &[usize]) -> bool {
if shape.is_empty() {
return true;
}
for (i, expected) in contiguous_strides(shape).enumerate() {
if expected != strides[i] {
return false;
}
}
true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The double .rev()
cancel out for lazy iterator operations. Also, the first computed stride value should be 1.
I changed the suggested contiguous_strides
implementation to match these criteria.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3083 +/- ##
==========================================
- Coverage 81.16% 81.09% -0.08%
==========================================
Files 815 817 +2
Lines 117201 117320 +119
==========================================
+ Hits 95121 95135 +14
- Misses 22080 22185 +105 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
/// such that the strides are in non-increasing order, and the stride at position | ||
/// `k` is equal to the product of the shapes of all dimensions greater than `k`. | ||
/// Axes with a shape of 1 are ignored. | ||
pub fn is_contiguous(shape: &[usize], strides: &[usize]) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that strategy actually, we could even create a function that could be used to create the strides AND be used for the checks:
pub fn contiguous_strides(shape: &[usize]) -> impl Iterator<Item = usize> {
let mut current = 1;
shape
.iter()
.rev()
.map(|val| {
current *= val;
current
})
.rev()
}
pub fn is_contiguous(shape: &[usize], strides: &[usize]) -> bool {
if shape.is_empty() {
return true;
}
for (i, expected) in contiguous_strides(shape).enumerate() {
if expected != strides[i] {
return false;
}
}
true
Checklist
run-checks all
script has been executed.Related Issues/PRs
Following this comment in #3077
Changes
Migrated
is_contiguous
to common (including minor fix).Testing
Also added a unit test for shape
[3, 1]
and strides[1, 1]
which would previously be marked as not contiguous.