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

Allow readonly arrays for higuchi_fd #13

Merged
merged 1 commit into from
Nov 20, 2021
Merged

Allow readonly arrays for higuchi_fd #13

merged 1 commit into from
Nov 20, 2021

Conversation

jvdd
Copy link
Contributor

@jvdd jvdd commented Nov 18, 2021

The current behavior of this method changes the datatype of x as np.asarray is a wrapper for np.array where copy=False. (see here)

I believe that this is (kind of) unexpected behavior, e.g., a user would not expect that the datatype would change when calculating a feature. Therefore, I suggest giving the user the option of not changing the datatype by adding a copy flag to the higuchi_fd function parameters. By default this flag = False, resulting in the same behavior as now (i.e., datatype of x is changed).

When benchmarking the speed of the code, I observed no real difference. Perhaps we should even remove the flag and just use np.array instead of np.asarray?

In [11]: x = np.random.rand(10_000).astype("float32")

In [12]: %timeit ant.higuchi_fd(x)
246 µs ± 5.24 µs per loop (mean ± std. dev. of 7 runs, 1000 loops each)

In [13]: x = np.random.rand(10_000).astype("float32")

In [14]: %timeit ant.higuchi_fd(x, copy=True)
242 µs ± 93.4 ns per loop (mean ± std. dev. of 7 runs, 1000 loops each)

PS: I really like the fast functions in this library 😄

@raphaelvallat raphaelvallat added the enhancement New feature or request label Nov 18, 2021
@raphaelvallat
Copy link
Owner

Hi @jvdd,

Thanks for opening the issue! Antropy forces x to be float64 (because of Numba compiling), so regardless of whether the array is copied or not the output is always going to be a float64:

@jit('float64(float64[:], int32)')

@jvdd jvdd closed this Nov 19, 2021
@jvdd
Copy link
Contributor Author

jvdd commented Nov 19, 2021

Hi @raphaelvallat,

Indeed, you are completely right. np.asarray returns a new array when it is necessary (i.e., when the datatype is not float64), whereas np.array always makes a copy (by default).

I made a mistake when narrowing down my error; I believed it was due the conversion not happening correctly when the type is not float64 (as you can see in my comment above). But, when reproducing this error, I noticed that the type conversion did not need to happen (as my data is of type float64).

So. the error I got was because the arrays I use are not writable. I believe working with non-writable arrays is a nice way of avoiding unexpected behavior (overriding values in your original data). This is minimal code that reproduces the error;

In [1]: import numpy as np; import antropy as ant                                                       

In [2]: x = np.arange(10_000).astype("float32") 
   ...: x.flags.writeable = False 
   ...: ant.higuchi_fd(x)                                                                               
Out[2]: 1.0003936754393057

In [3]: x = np.arange(10_000).astype("float64") 
   ...: x.flags.writeable = False 
   ...: ant.higuchi_fd(x)                                                                               
---------------------------------------------------------------------------
TypeError   
    ...
TypeError: No matching definition for argument type(s) readonly array(float64, 1d, C), int64

=> I rebased the master and added a different commit that now only changes the @jit signature of the _higuchi_fd function. Now readonly & non-readonly arrays are supported 😄

@jvdd jvdd reopened this Nov 19, 2021
@jvdd jvdd changed the title Add copy flag to higuchi_fd function Support readonly arrays for higuchi_fd Nov 19, 2021
@jvdd jvdd changed the title Support readonly arrays for higuchi_fd Allow readonly arrays for higuchi_fd Nov 19, 2021
@raphaelvallat
Copy link
Owner

Got it — merging now! Thanks for the PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants