- Notifications
You must be signed in to change notification settings - Fork338
Addshrink_to_fit
toArray
#1141
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
src/impl_owned_array.rs Outdated
/// assert_eq!(a, b); | ||
/// ``` | ||
pub fn shrink_to_fit(&mut self) { | ||
self.to_default_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.
to default stride is an expensive operation - so we can probably avoid it on an array that is already perfectly fit.
We do prefer to not lean too strongly on a default memory order if possible. Any contiguous array should not have its memory order thrown around. I would even prefer if an array keeps its memory order the same, if it can.
src/impl_owned_array.rs Outdated
/// Convert from any stride to the default stride | ||
pub fn to_default_stride(&mut self) { | ||
let view_mut = |
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'm not sure about all the details of this method, but I think it's better to take a step back and see if there isn't already some functionality that can do this - maybe .to_shape()?
shrink_to_fit is a good idea - should it modify array memory layout? When should it modify? Some more description could help us get a good design. |
Uh oh!
There was an error while loading.Please reload this page.
bokutotu commentedDec 24, 2021 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for your the code review 😀
For example, I think that changing the memory order is necessary in the following cases. let a =array![[[0,1,2],[3,4,5],[6,7,8]],[[9,10,11],[12,13,14],[15,16,17]],[[18,19,20],[21,22,23],[24,25,26]]];let a = a.slice_move(s![..,1.., ..]);// a ->// [[[3, 4, 5],// [6, 7, 8]],//// [[12, 13, 14],// [15, 16, 17]],//// [[21, 22, 23],// [24, 25, 26]]], shape=[3, 2, 3], strides=[9, 3, 1], layout=c (0x4), const ndim=3 I think the memory order would be as follows if we apply before shrink_to_fit
after shrink_to_fit
On the other hand, in the following case, it looks like you can do let a =array![[[0,1,2],[3,4,5],[6,7,8]],[[9,10,11],[12,13,14],[15,16,17]],[[18,19,20],[21,22,23],[24,25,26]]];let a = a.slice_move(s![..2, .., ..]);// a ->// [[[0, 1, 2],// [3, 4, 5],// [6, 7, 8]],//// [[9, 10, 11],// [12, 13, 14],// [15, 16, 17]]], shape=[2, 3, 3], strides=[9, 3, 1], layout=Cc (0x5), const ndim=3 I think the only time you can apply |
I'm back next week |
Apparently not back so fast, I apologize. |
Don't worry about it. We all have busy times 😀 |
What is the current status of this PR? |
Well, I want to come back to this and it would be the first thing I'd work on in ndarray |
@bokutotu If I you are still interested, I would like to try to pick this up again. What would be really helpful would be to remove the formatting-related part of the diff, just so that is easier to review. Other than that, I suspect that all of the feedback provided so far has already been addressed. If you do reboot this, please try to ignore all the Clippy and deprecation issues for now. We will have to get the master branch into shape elsewhere. |
@adamreichold Thanks for your reply. I would like to make a commit to delete the diffs once they are out in cargo fmt. Please wait a bit as work is taking up a lot of my time right now. |
Uh oh!
There was an error while loading.Please reload this page.
#427
shrink_to_fit
toArray<A, D>
to_default_strid
toArray<A, D>
offset_from_data
toArray<A, D>
shrink_to_fit
toOwnedRepr