-
Notifications
You must be signed in to change notification settings - Fork 96
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
upgrade ndarray to 0.16 #569
Conversation
Thanks, but I don't think this is right (just as the previous code wasn't right). For reference, I saw this example in rust-ndarray/ndarray#994: let a = Array2::<f64>::zeros([3, 4]).slice_move(s![1.., ..]);
let b = Array2::<f64>::zeros([2, 4]);
assert!(a.is_standard_layout());
assert!(b.is_standard_layout());
assert_eq!(a.shape(), b.shape());
assert_eq!(a.strides(), b.strides());
assert_ne!(
unsafe { a.as_ptr().offset_from(a.into_raw_vec().as_ptr()) }, // 4
unsafe { b.as_ptr().offset_from(b.into_raw_vec().as_ptr()) }, // 0
); Which implies that slicing sometimes avoids copying the data by bumping an offset from the start of the storage (the example strips off the first row). So the |
I understood having a standard layout means also having an offset of 0, but I am afraid you are right. So we need to copy the data (like it is currently done for the non-standard-layout) in case of an offset. I will add this to the PR later today. |
I'm again wondering if we should fully switch to |
src/raster/buffer.rs
Outdated
if let Some(offset) = offset { | ||
data.into_iter().skip(offset).collect() | ||
} else { | ||
data |
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.
Let's also handle offset == 0
the same way.
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.
Done - I removed the if
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.
That might do an extra copy in the zero offset case.
src/raster/buffer.rs
Outdated
value.into_raw_vec() | ||
let (data, offset) = value.into_raw_vec_and_offset(); | ||
if let Some(offset) = offset { | ||
data.into_iter().skip(offset).collect() |
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.
data.into_iter().skip(offset).collect() | |
data[offset].to_vec() |
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.
Yepp, done
229daaf
to
d1fa058
Compare
d1fa058
to
ed28a25
Compare
@rouault looks like I've just won the lottery in https://github.com/georust/gdal/actions/runs/11372075639/job/31635584318:
|
This PR upgrades the ndarray dependency from 0.15 to 0.16
CHANGES.md
if knowledge of this change could be valuable to users.