ENH: add optional argument dtype to numpy.fft.fftfreq (#9126)
#16496
Conversation
|
Should we cast |
|
We could do that, before using it to compute EDIT: @jakirkham thoughts? |
|
It's worth noting that even after the switch to pocketfft, I just wanted to comment here quickly, even if the mailing list might be the more appropriate forum for this discussion. |
No strong thoughts. More of a question. Though you're right. This may not be needed. |
|
LGTM. Thoughts from others? |
| if dtype != float32: | ||
| dtype = float64 | ||
|
|
||
| val = reciprocal(n*d, dtype=dtype) | ||
| N = n//2 + 1 | ||
| results = arange(0, N, dtype=int) |
There was a problem hiding this comment.
Sorry one last thought. Would it make sense to cast this to dtype as well?
| results = arange(0, N, dtype=int) | |
| results = arange(0, N, dtype=dtype) |
There was a problem hiding this comment.
we could but from what I understand this is always just an array of integers, which when multiplied with the dtype cast val will lead to the returned result being of type dtype anyway, right?
that's a very good point @rossbar, thanks for bringing it up. To be honest I haven't really considered it this way; I saw that this was an open issue for quite some time, so I thought I would attempt to work on it. But considering the |
Deals with issue #9126 requesting the addition of dtype argument option for
fftfreqandrfftfreqfunctions. To do so, I added an optional argument and used thereciprocalfunction to set the type of the array that will return the result.A different way to do so would be to leave the calculation as is and instead modify the return line so that it reads
return asarray(results*val, dtype=dtype); I think the two ways should be equivalent in terms of the end result, but I was hoping for some feedback.I've submitted a PR draft since I'm not entirely sure how to do the check that the required
dtypeis one of the numpy float dtypes; I'm using theisinstancefunction to check, but I'm not sure what would be the proper types to check against and where to import those from. Any additional comments are of course welcome!EDIT: I'm now allowing the output array type to be either float32 or float64, with float64 being the default behavior (as before). Let me know what you think; I'm changing the PR from draft to open.
cc @jakirkham @mhvk @eric-wieser @leofang @charris
Closes #9126