Support zero-length axis in .map_axis/_mut()#612
Support zero-length axis in .map_axis/_mut()#612jturner314 merged 6 commits intorust-ndarray:masterfrom andrei-papou:zero-length-axis-in-map-axis
axis in .map_axis/_mut()#612Conversation
src/impl_methods.rs
Outdated
| } else { | ||
| let smaller_zero_dim = <D::Smaller as Dimension>::zeros(self.dim.ndim() - 1); | ||
| let ptr = std_ptr::NonNull::<A>::dangling().as_ptr(); | ||
| unsafe { |
There was a problem hiding this comment.
I'd like a comment here to explain why this is a safe thing to do (same in the other spot).
There was a problem hiding this comment.
@LukeMathWalker is it enough to point to this section in the ArrayBase docstring?
The only exceptions are if the array is empty or the element
type is zero-sized. In these cases,ptrmay be dangling, but it must
still be safe to [.offset()] the pointer along the axes.
There was a problem hiding this comment.
I'd suggest to extract it in a function, so that we can reuse code across the two calling sites and keep a better eye on it. It's probably better to copy the relevant section over, to have it close to the code.
jturner314
left a comment
There was a problem hiding this comment.
Thanks for working on this @andrei-papou! Unfortunately, the implementation isn't quite right. (See my comment on the code.) Here's one possible solution:
pub fn map_axis<'a, B, F>(&'a self, axis: Axis, mut mapping: F)
-> Array<B, D::Smaller>
where D: RemoveAxis,
F: FnMut(ArrayView1<'a, A>) -> B,
A: 'a,
S: Data,
{
let view_len = self.len_of(axis);
let view_stride = self.strides.axis(axis);
if view_len == 0 {
let new_dim = self.dim.remove_axis(axis);
Array::from_shape_fn(new_dim, move |_| mapping(ArrayView::from(&[])))
} else {
// use the 0th subview as a map to each 1d array view extended from
// the 0th element.
self.index_axis(axis, 0).map(|first_elt| {
unsafe {
mapping(ArrayView::new_(first_elt, Ix1(view_len), Ix1(view_stride)))
}
})
}
}I'd like to see some tests for the zero-length-axis case as part of the PR too.
tests/array.rs
Outdated
| counter | ||
| }); | ||
| assert_eq!(result.shape(), &[3, 4]); | ||
| assert_eq!(result, arr2(&[ |
There was a problem hiding this comment.
Overall, this test is good, but we don't guarantee the iteration order of map_axes, so the elements of result are not guaranteed to be arranged in this way. Something like
itertools::assert_equal(result.iter().cloned().sorted(), 1..=3*4);would work to verify that the elements are correct while ignoring their ordering.
(For access to .sorted(), make sure to use itertools::Itertools and add features = ["use_std"] to the itertools dependency in Cargo.toml.)
There was a problem hiding this comment.
Thanks, I've updated the test accordingly.
Merge main rep master to fork master
…ngth-axis-in-map-axis
|
@jturner314 any more comments on the changes? the issues above are fixed |
|
Looks good to me. Thanks! |
PR for #579