implement get_row_* for Matrix, MatrixSlice and MatrixSliceMut

[tafia]

• get_row
• get_row_mut
• get_row_unchecked
• get_row_mut_unchecked

Note:
For MatrixSlices, get_row and get_row_unchecked are implemented
directly on BaseSlice trait, as it already provides all necessary methods.

Similarly, moved get_unchecked slice implementations directly into the
trait, I don't really know but this may be a breaking change (I can revert this part if necessary).

[tafia]

close #7

[AtheMathmo]

Overall this is really good - just a few minor things.

Instead of

fn get_row(&self, index: usize) -> Option<&[T]> {

    if index >= self.rows() {
        return None;
    }

    unsafe { Some(self.get_row_unchecked(index)) }
}

I think it would be more rust like to use:

fn get_row(&self, index: usize) -> Option<&[T]> {
    if index < self.rows() {
        unsafe { Some(self.get_row_unchecked(index)) }
    } else {
        None
    }
}

I swapped the comparison out of personal preference - I wont hold it against you if you switch back :).

And also I'd appreciate if you can add some tests too. Just check that the unchecked version return the correct row in a basic test. And then check that the checked version also returns a valid Some option, or a None when applicable. Also testing that mutability works for get_row_mut would be appreciated - I've broken that a few times when I've moved things around so try and remember to add tests now... If you don't have time I'm happy to add these myself.

The actual code itself here is good (I believe) - I just want the tests to help prevent breakage if we move things around in future.

Similarly, moved get_unchecked slice implementations directly into the
trait

You're right - I think this is a good change!

[AtheMathmo]

Actually - I just noticed that you already have tests in the documentation to cover most things I mentioned. Sorry for the oversight. If you could just make that one nitpick change about the if {...} else {...} block I'll be happy to merge and then release this.

[tafia]

Done.

I just noticed that you already have tests in the documentation to cover most things

Yes. The only missing part was for the BaseSlice implementations (there were no examples for the other functions).
I've added some examples here too.

[tafia]

I have another doubt: shoudn't we have:

fn get_row(&self, index: usize) -> Option<MatrixSlice>;
fn get_row_mut(&self, index: usize) -> Option<MatrixSliceMut>;

[AtheMathmo]

This question has come up before and I opted not to do this.

The main reasoning is that &[T] guarantees contiguous data which allows us to optimize some operations. This isn't true of MatrixSlice. Right now I haven't run into a case where I need to treat a row as a matrix in itself (I think). Maybe I'll change this later though.

I'm happy to merge this now - but I'll give you some time to respond in case you disagree.

[tafia]

I'm fine.
I think the best would be to have kind of a VectorSlice instead (that
guarantees continuous memory as well).
VectorSlice: Deref<[T]> or something like that.
But this would be a too big change, let's do this later if truly needed.

On 26 Jul 2016 22:18, "James Lucas" [email protected] wrote:

This question has come up before and I opted not to do this.

The main reasoning is that &[T] guarantees contiguous data which allows us
to optimize some operations. This isn't true of MatrixSlice. Right now I
haven't run into a case where I need to treat a row as a matrix in itself
(I think). Maybe I'll change this later though.

I'm happy to merge this now - but I'll give you some time to respond in
case you disagree.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#8 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHAszgyiQG3GcSEFbdWWUwLEbQJgBLd2ks5qZhc4gaJpZM4JUuk4
.

[AtheMathmo]

Yes I agree - that may be a reasonable way to achieve this. I'll merge now and release this as 0.2.1 shortly.

Thanks again!