Add accumulate_axis_inplace method and implement NdProducer for RawArrayView/Mut #611
Conversation
|
Amazing, we'll need this to improve upon .. so many things, including how we handle uninitalized values. I'll need to remind myself to come back to this. |
| prev.slice_axis_inplace(axis, Slice::from(..-1)); | ||
| let mut curr = self.raw_view_mut(); | ||
| curr.slice_axis_inplace(axis, Slice::from(1..)); | ||
| // This implementation relies on `Zip` iterating along `axis` in order. |
There was a problem hiding this comment.
Good point, I guess we'll think about if that's something we can guarantee. Probably not publically.
bluss
force-pushed
the
accumulate-axis-inplace
branch
from
3faaa7e
to
7b87d5a
Compare
|
Rebased to resolve the conflict. I had to reformat and fix some things for NonNull. I added a commit on top with what I think would be the careful way to use raw view (see commit log). I haven't studied the unsafe coding guidelines that well, so this is just my rough understanding. |
|
Updating because RawView + Zip should be an awesome ally in solving fundamentals, like how to do more operations in place or without excessive copying, or just to handle uninitialized data better. |
|
The format checker wants to ruin the formatting of the arrays in tests, and I'd like to make it less strict :/ |
bluss
force-pushed
the
accumulate-axis-inplace
branch
from
c950cc0
to
c5655cd
Compare
We had: 1. let ptr1 = self.raw_view(); // Borrow &self 2. let ptr2 = self.raw_view_mut(); // Borrow &mut self 3. Use ptr1 and ptr2 While I'm not an expert at the unsafe coding guidelines for Rust, and there are more places in ndarray to revisit, I think it's best to change change 1 and 2 - they don't pass my internalized borrow checker. It seems as though the steps 1, 2, 3 could be against the rules as ptr1 is borrowed from the array data, and its scope straddles the mut borrow of the array data in 2. For this reason, I think this would be better: 1. let ptr2 = self.raw_view_mut() // Borrow &mut self 2. let ptr1 = derive from ptr2 3. use ptr1 and ptr2 RawView should hopefully be our ally in making a better ndarray from the foundation.
bluss
force-pushed
the
accumulate-axis-inplace
branch
from
c5655cd
to
886b6a1
Compare
| /// ], | ||
| /// ); | ||
| /// ``` | ||
| pub fn accumulate_axis_inplace<F>(&mut self, axis: Axis, mut f: F) |
There was a problem hiding this comment.
This method is a neat demonstration of an array Zip-ing with itself. It seems a bit niche but we can of course include it. But the question of course goes out, how do we generalize this. Apart from the raw pointer Zip, of course? How to generalize the Self-zip in a safe Rust package?
Apart from returning to the idea of an array view of Cells, which is not hard to do.
There was a problem hiding this comment.
I think we really need to wait until Rust has generic associated types before we can write good self-Zip abstractions. We can do things like this today:
/// Folds over the mutable windows of the specified size.
pub fn windows_mut_fold<E, B, F>(&mut self, window_size: E, init: B, f: F)
where
E: IntoDimension<Dim = D>,
F: for<'a> FnMut(B, ArrayViewMut<'a, A, D>) -> B,
S: DataMut,
{
// ...
}but without generic associated types, we can't abstract this into a WindowsMut producer that implements a trait, because the item type (which is an associated type of the trait) has a temporary lifetime.
| prev.slice_axis_inplace(axis, Slice::from(..-1)); | ||
| curr.slice_axis_inplace(axis, Slice::from(1..)); | ||
| // This implementation relies on `Zip` iterating along `axis` in order. | ||
| Zip::from(prev).and(curr).apply(|prev, curr| unsafe { |
There was a problem hiding this comment.
This is a particularly simple Zip - the prev and curr pointer are always just the stride of axis apart, and in that sense it could have been just a unary loop, with one pointer prev and another prev + offset. If the Zip only compiles to the equivalent, all the better, then we have a nice way to express exactly that. (But I guess Zip has some kind of overhead here.)
|
Thanks for this, the ndproducer impls will be very useful going forward. And I guess we should have discussions about how to use it correctly (and eventually produce docs for that when we know 🙂 ) |
This
accumulate_axis_inplacemethod is a non-cloning version of theaccumulate_axis_methodproposed in #513.