Skip to content
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

Merged
merged 7 commits into from
Jun 14, 2018
Merged

Conversation

LukeMathWalker
Copy link
Member

@LukeMathWalker LukeMathWalker commented May 29, 2018

Following #452 I tried to implement map_mut and map_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:

  • I have implemented TrustedIterator for IterMut and slide::IterMut - was there any reason this was not already in place (apart from not having been needed so far)?
  • to avoid issues with the borrow checker in the else branch I have modified
if let Some(slc) = as_slice_memory_order_mut() {
    [...]
} else {
    [...]
}

into

if self.is_contiguous() {
    let slc = self.as_slice_memory_order_mut().unwrap();
    [...]
} else {
    [...]
}

which seemed reasonable looking at as_slice_memory_order_mut implementation:

    /// Return the array’s data as a slice if it is contiguous,
    /// return `None` otherwise.
    pub fn as_slice_memory_order_mut(&mut self) -> Option<&mut [A]>
        where S: DataMut
    {
        if self.is_contiguous() {
            self.ensure_unique();
            unsafe {
                Some(slice::from_raw_parts_mut(self.ptr, self.len()))
            }
        } else {
            None
        }
    }

@jturner314 @rcarson3

@LukeMathWalker LukeMathWalker mentioned this pull request Jun 2, 2018
Copy link
Member

@bluss bluss left a 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

@@ -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>
Copy link
Member

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.

Copy link
Member Author

@LukeMathWalker LukeMathWalker Jun 13, 2018

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

Copy link
Member

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.

Copy link
Member Author

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.

{
let dim = self.dim.clone();
let strides = self.strides.clone();
if self.is_contiguous() {
Copy link
Member

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.

-> Array<B, D::Smaller>
where D: RemoveAxis,
F: FnMut(ArrayViewMut1<'a, A>) -> B,
A: 'a + Clone,
Copy link
Member

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

Copy link
Member

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.

/// 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,
Copy link
Member

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.)

Copy link
Member

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)))
}
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good!

@LukeMathWalker
Copy link
Member Author

Thanks for reviewing @bluss!
I should have implemented/fixed all lines you pointed to (as well as restoring trailing whitespaces). :D

@bluss
Copy link
Member

bluss commented Jun 14, 2018

Thanks a lot for this!

@bluss bluss merged commit 7ac0600 into rust-ndarray:master Jun 14, 2018
@LukeMathWalker LukeMathWalker deleted the map_axis_mut branch June 15, 2018 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants