split_at for Array #94
Conversation
|
I think I prefer if we only provide axis split (not outer split, since it's a simple special case, isn't it?) |
|
|
||
| let mut dim_left = self.dim.clone(); | ||
| dim_left.slice_mut()[axis] = index; | ||
| let left = ArrayView { |
There was a problem hiding this comment.
You can use ArrayView::new_ here.
There was a problem hiding this comment.
right, that's less boilerplate
|
Indeed it's just a special case, but I was wondering if we didn't want to have some symmetry with the |
|
The most flexible API is not the easiest to use. Again a view based Consider a function like this:
The only way to pass on a view with the same lifetime This is the same way |
|
Consistency is good, but we need to stem the rising tide of a million methods too. Maybe I shouldn't have added |
|
Yes I see your point with the lifetimes issue. I wanted to mimick the slice's API but I guess there is some special treatment to slices here. One (probably ugly) way I found to circumvent that problem in other crates (sprs) was to have additional methods defined only on views which would return views based on the lifetime of the data, not the lifetime of self in the method call. I wish there were some sort of "lifetime specialization" to overcome this issue. Not sure I'm really clear here, my thoughts are a bit blurry and I think I lack the proper vocabulary to express them. |
|
Yes! Slices are dsts, so they implement methods on the type I think that |
If I remember correctly, I've had a similar issue in sprs, and the solution was to define the impl block on the full type, not the alias type. That was quite some time ago but it might be worth a try. |
|
Yes, that will work |
|
The for loop doesn't seem right. Is this right: consider a 3 x 4 x 5 array (3 pages, 4 rows, 5 columns) Split on axis = 2, index = 2 should produce two views of shapes |
|
I'm not sure I got your point here, that for loop is only supposed to help find the correct pointer offset, but the resulting shapes should be what you said. |
|
I'll review it more carefully. Some pen and paper doodling (that's when you know you're actually thinking..). |
|
I've switched to the view-by-value pattern and added some tests. I'll probably add a macro to factor out the code once again, and I need to test more complicated cases. |
|
Awesome |
| *ind = index; | ||
| } | ||
| } | ||
| let offset = self.dim.stride_offset_checked(&self.strides, |
There was a problem hiding this comment.
There's the stride_offset method which does this without bounds checking.
There was a problem hiding this comment.
On a closer look, we can just skip the whole for loop, can't we? Using stride * index directly (function stride_offset in dimension for example).
|
I've fixed the formatting issue, replaced the for loop by an appropriate call to I'm no longer sure I should try and factor both impls in a macro, they're quite short now. As you wish :) |
|
Awesome, this is good. I'll merge the Axis PR too. |
|
Before I release this, what do you think of renaming the methods to
versus
I bring this up since |
|
Yes |
|
Done. |
This PR introduces a
split_atlike family of functions for arrays. This is currently work in progress.Status:
axis_split_ataxis_split_at_mut