Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add try_into_owned_nocopy method #1022

Merged
merged 2 commits into from
Nov 7, 2021

Conversation

jturner314
Copy link
Member

@jturner314 jturner314 commented Jun 1, 2021

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.

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants