Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
bokutotu wants to merge7 commits intorust-ndarray:master
base:master
Choose a base branch
Loading
frombokutotu:add_shrink_to_fit

Conversation

bokutotu
Copy link

@bokutotubokutotu commentedDec 22, 2021
edited
Loading

#427

  • addshrink_to_fit toArray<A, D>
  • addto_default_strid toArray<A, D>
  • addoffset_from_data toArray<A, D>
  • addshrink_to_fit toOwnedRepr

/// assert_eq!(a, b);
/// ```
pub fn shrink_to_fit(&mut self) {
self.to_default_stride();
Copy link
Member

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.


/// Convert from any stride to the default stride
pub fn to_default_stride(&mut self) {
let view_mut =
Copy link
Member

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()?

@bluss
Copy link
Member

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.

@blussbluss changed the titleAddshrink_to fit toOwendRepr ArrayAddshrink_to_fit toArrayDec 23, 2021
@bokutotu
Copy link
Author

bokutotu commentedDec 24, 2021
edited
Loading

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 applyshrink_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 doshrink_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 applyshrink_to_fit without changing the memory order is when the dimension with the largest stride is changed, and there is no offset fromself.data.ptr toself.ptr. So I thinkshrink_to_fit without changing the memory order can only be used for quite limited cases.

@bokutotubokutotu requested a review fromblussDecember 27, 2021 10:57
@bluss
Copy link
Member

I'm back next week

bokutotu reacted with thumbs up emoji

@bluss
Copy link
Member

Apparently not back so fast, I apologize.

@bokutotu
Copy link
Author

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

@bokutotu
Copy link
Author

What is the current status of this PR?

@bluss
Copy link
Member

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

bokutotu reacted with thumbs down emoji

@adamreichold
Copy link
Collaborator

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

@bokutotu
Copy link
Author

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

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@blussblussAwaiting requested review from bluss

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@bokutotu@bluss@adamreichold

[8]ページ先頭

©2009-2025 Movatter.jp