ndarray icon indicating copy to clipboard operation
ndarray copied to clipboard

Add `shrink_to_fit` to `Array`

Open bokutotu opened this issue 3 years ago • 9 comments
trafficstars

#427

  • add shrink_to_fit to Array<A, D>
  • add to_default_strid to Array<A, D>
  • add offset_from_data to Array<A, D>
  • add shrink_to_fit to OwnedRepr

bokutotu avatar Dec 22 '21 10:12 bokutotu

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.

bluss avatar Dec 22 '21 19:12 bluss

Thanks for your the code review 😀

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.

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 shrink_to_fit to this array.

before shrink_to_fit

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

after shrink_to_fit

3 4 5 6 7 8 12 13 14 15 16 17 21 22 23 24 25 26

On the other hand, in the following case, it looks like you can do shrink_to_fit without changing the memory order, and just release a part of the heap.

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 shrink_to_fit without changing the memory order is when the dimension with the largest stride is changed, and there is no offset from self.data.ptr to self.ptr. So I think shrink_to_fit without changing the memory order can only be used for quite limited cases.

bokutotu avatar Dec 24 '21 13:12 bokutotu

I'm back next week

bluss avatar Dec 30 '21 22:12 bluss

Apparently not back so fast, I apologize.

bluss avatar Feb 14 '22 20:02 bluss

Don't worry about it. We all have busy times 😀

bokutotu avatar Feb 18 '22 10:02 bokutotu

What is the current status of this PR?

bokutotu avatar Jun 02 '22 12:06 bokutotu

Well, I want to come back to this and it would be the first thing I'd work on in ndarray

bluss avatar Jul 23 '22 18:07 bluss

@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 avatar May 23 '23 19:05 adamreichold

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

bokutotu avatar May 24 '23 06:05 bokutotu