8000 Add try_into_owned_nocopy method by jturner314 · Pull Request #1022 · rust-ndarray/ndarray · GitHub
[go: up one dir, main page]
Skip to content

Add try_into_owned_nocopy method#1022

Merged
bluss merged 2 commits intorust-ndarray:masterfrom
jturner314:try_into_owned
Nov 7, 2021
Merged

Add try_into_owned_nocopy method#1022
bluss merged 2 commits intorust-ndarray:masterfrom
jturner314:try_into_owned

Conversation

@jturner314
Copy link
Member
@jturner314 jturner314 commented Jun 1, 2021 •
edited
Loading

I've been working on improvements to the layout handling in ndarray-linalg to make the code easier to understand, fix bugs, and avoid unnecessary cloning and by-clone transposes. One of operations I need to implement is, "Convert an ArrayBase into a LAPACK-compatible Array (i.e. strides = [1, s], with s >= nrows), while avoiding unnecessary cloning." I don't think it's possible to implement this today without adding more methods to ArrayBase. To address this, I propose adding a try_into_owned_nocopy method which is similar to into_owned but returns Err(self) if the data would need to be cloned, instead of actually performing the cloning. This way, if the data needs to be cloned, I can clone it into an array of the correct layout, like this:

match array.try_into_owned_nocopy() {
    Ok(o) if is_lapack_compatible(&o) => o,
    Ok(o) => clone_into_new_lapack_array(&o),
    Err(a) => clone_into_new_lapack_array(&a),
}

There are other ways this problem could be solved, but try_into_owned_nocopy is the best I came up with.

A couple of notes about the PR:

  1. If we implement DataOwned for CowRepr in another PR, then I'd change the bound on try_into_owned_nocopy to be S: DataOwned, since there wouldn't be any reason to call it on any non-DataOwned types. Edit: I was wrong; the method is actually useful for S: Data after all, and there isn't a strong reason to restrict it to S: DataOwned anyway.

  2. I'm not completely satisfied with the name try_into_owned_nocopy. Other suggestions would be welcome.

@jturner314
Use type aliases to simplify type signatures
061ffce 
Clippy warned that some of the types were too complex, so this
commit simplifies those lines and a few others.
@bluss bluss added this to the 0.15.4 milestone Jun 5, 2021
@bluss
Copy link
Member
bluss commented Nov 6, 2021

We want to merge this right? I think the name will be ok

@jturner314
Copy link
Member Author
jturner314 commented Nov 6, 2021

Yes, I still think this is a good idea, and I agree that the name is good enough.

@bluss bluss merged commit daaf625 into rust-ndarray:master Nov 7, 2021
@bluss
Copy link
Member
bluss commented Nov 7, 2021

Great, thanks!

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

Reviewers

No reviews

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

0.15.4

Development

Successfully merging this pull request may close these issues.

2 participants

@jturner314 @bluss
0