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

Remove unneeded copying. #12

Merged
merged 3 commits into from
Feb 7, 2020

Conversation

danielballan
Copy link
Member

@danielballan danielballan commented Nov 15, 2019

When algorithm operates by modifying the input array in place, it is
good manners to make a copy first so that the function does not mutate
user input. I am guessing that that was the motivation for the use of
copy() here.

However, as far as I can tell, this algorithm does not modify signal
(or its alias a) in place. It reassigns the variable a in a loop,
but does not actually modify the input array object.

Therefore, we can remove this copying and increase the speed. Also, note
that the call to np.array explicitly cast the input to a literal numpy
array. By removing that from the code, we accept dask arrays and cupy
arrays and sparse arrays and allow them to flow through the algorithm via the NEP-18 numpy
dispatch mechanism. (See this section of the numpy documentation,
written by me as it happens, for details.)

attn @leofang @aryabhatt

When algorithm operates by modifying the input array in place, it is
good manners to make a copy first so that the function does not mutate
user input. I am guessing that that was the motivation for the use of
`copy()` here.

However, as far as I can tell, this algorithm does not modify `signal`
(or its alias `a`) in place. It reassigns the variable `a` in a loop,
but does not actually modify the input array object.

Therefore, we can remove this copying and increase the speed. Also, note
that the call to `np.array` explicitly cast the input to a literal numpy
array. By removing that from the code, we accept dask arrays and cupy
arrays and allow them to flow through the algorithm via the NEP-18 numpy
dispatch mechanism.
@leofang
Copy link

leofang commented Feb 7, 2020

Sorry I was digging out old GitHub notifications from my inbox and just saw this. It'd be great to have some sort of NEP guarantee. If this works, #10 is likely no longer needed, and users could just do this?

import cupy as cp
import autocorr

data = # a cupy array
g1, tau = autocorr.multitau(data)

@danielballan danielballan mentioned this pull request Feb 7, 2020
@danielballan
Copy link
Member Author

@leofang That's my hope! It doesn't quite work as is with sparse arrays, but it's close, and I think we can get there. See #14 for my work in progress on that. Likely it would be the same situation for cupy. It does work as is with dask, though, which is cool.

I am going to self-merge this; @aryabhatt gave me the go-ahead on a call.

@danielballan danielballan merged commit cc939bb into scikit-beam:master Feb 7, 2020
@danielballan danielballan deleted the remove-unneeded-copy branch February 7, 2020 19:33
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