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 mapv_into_any(), resolves #1031 #1040

Merged
merged 1 commit into from
Jul 9, 2021
Merged

Conversation

benkay86
Copy link
Contributor

@benkay86 benkay86 commented Jun 25, 2021

First attempt at pull request for #1031.

Since Rust does not yet have specialization, mapping between generic numerical types in ndarray is challenging when the types might be different. For example, we might want to perform on operation on an array of real numbers to get an array that might be real or might be complex. We can do this now with ArrayBase::mapv(), but if it turns out that both the input and output are the same type (e.g. real numbers) then mapv() will perform unnecessary allocation of a new array compared to mapv_into().

This PR proposes a generic method ArrayBase::mapv_into_any() whose signature accepts a closure mapping between different types FnMut(A) -> B but which uses memory-efficient mapv_into() when A and B are the same type. The type comparison is performed at runtime. This is the most memory-efficient way to map between arrays of possibly different types using stable Rust.

The use of unsafe is discussed on the Rust language forum.

@benkay86
Copy link
Contributor Author

benkay86 commented Jul 7, 2021

Thank you for granting permission to run git workflows on this pull request. It looks like clippy on the beta channel rejects a large portion of the ndarray codebase, but it appears that this pull request has passed all checks.

@bluss
Copy link
Member

bluss commented Jul 9, 2021

It just looks like clippy found new lints in a new release

src/impl_methods.rs Outdated Show resolved Hide resolved
Copy link
Member

@bluss bluss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid to me, should be useful

@bluss
Copy link
Member

bluss commented Jul 9, 2021

Please use issue closing keywords in the pr description so that the linked issue is closed when this is merged https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@benkay86
Copy link
Contributor Author

benkay86 commented Jul 9, 2021

Fixed commit comment.

Edit: Looks like you'll still need to manually link to issue #1031, sorry. I don't appear to have permission.

@bluss
Copy link
Member

bluss commented Jul 9, 2021

FWIW, we already use this kind of ad-hoc specialization in ndarray with the linalg code, so it feels familiar to us.

@benkay86 benkay86 changed the title add mapv_into_any() for #1031 add mapv_into_any(), resolves #1031 Jul 9, 2021
@bluss bluss added this to the 0.15.4 milestone Jul 9, 2021
@bluss
Copy link
Member

bluss commented Jul 9, 2021

(milestone - not known if we will have 0.15.4 or 0.16 atm). Thanks!

@bluss bluss merged commit 000ea7f into rust-ndarray:master Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants