-
Notifications
You must be signed in to change notification settings - Fork 314
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
Add map_mut and map_axis_mut #460
Conversation
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.
Nice, just small things to fix
src/impl_methods.rs
Outdated
@@ -930,7 +930,7 @@ impl<A, S, D> ArrayBase<S, D> where S: Data<Elem=A>, D: Dimension | |||
/// [6, 6, 7, 7, 8, 8, 0], | |||
/// [6, 6, 7, 7, 8, 8, 0]])); | |||
/// ``` | |||
pub fn exact_chunks_mut<E>(&mut self, chunk_size: E) -> ExactChunksMut<A, D> | |||
pub fn exact_chunks_mut<E>(&mut self, chunk_size: E) -> ExactChunksMut<A, D> |
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.
Please avoid formatting changes outside the code you are editing.
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.
My vim settings removes trailing whitespaces on save... Got to change it when I am working on ndarray :P
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 have Emacs set up the same way. Fwiw, I've found it useful to have the "save" keybinding set to remove trailing whitespace, but preserve the ability to call the save-buffer
command directly if desired to save without removing whitespace.
Anyway, I've submitted PR #464 to remove all the trailing whitespace in ndarray
so that we won't have to worry about this issue anymore.
For the purpose of this PR, though, @bluss is right that the PR should not reformat lines unrelated to the changes.
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.
Yes, I was thinking about doing something similar to that in my .init.vim
.
Until #464 lands I guess I'll be restoring whitespaces once the code has been approved and it is just waiting to be merged - final clean-up, so to speak.
src/impl_methods.rs
Outdated
{ | ||
let dim = self.dim.clone(); | ||
let strides = self.strides.clone(); | ||
if self.is_contiguous() { |
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.
This redundant is_contiguous call is unfortunate. The strides.clone call can move inside the first if branch though, so that's nice.
src/impl_methods.rs
Outdated
-> Array<B, D::Smaller> | ||
where D: RemoveAxis, | ||
F: FnMut(ArrayViewMut1<'a, A>) -> B, | ||
A: 'a + Clone, |
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.
This A: Clone here is unwanted
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 'a
here is good.
src/impl_methods.rs
Outdated
/// Return an array with the same shape as `self`. | ||
pub fn map_mut<'a, B, F>(&'a mut self, f: F) -> Array<B, D> | ||
where F: FnMut(&mut A) -> B, | ||
A: 'a, |
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 'a
parameter now has no use and can be removed. (It having no use unlike the &self
version of map, is correct.)
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.
Hm.. wait a minute, that's not right, using 'a
the same way as the &self
map
would be good.
unsafe { | ||
mapping(ArrayViewMut::new_(first_elt, Ix1(view_len), Ix1(view_stride))) | ||
} | ||
}) |
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.
This looks good!
Thanks for reviewing @bluss! |
Thanks a lot for this! |
Following #452 I tried to implement
map_mut
andmap_axis_mut
.There is unsafe code in multiple spots and my familiarity with the overall architecture of ndarray (and Rust in general) is still lacking, so I'd like to understand if some of my choices might be calling for disaster in disguise:
TrustedIterator
forIterMut
andslide::IterMut
- was there any reason this was not already in place (apart from not having been needed so far)?into
which seemed reasonable looking at
as_slice_memory_order_mut
implementation:@jturner314 @rcarson3