Conversation
jcrist
left a comment
jcrist
left a comment
There was a problem hiding this comment.
Overall this seems fine to me.
dask/array/fft.py
Outdated
| scipy = None | ||
|
|
||
| from dask.array.creation import linspace as _linspace | ||
| from dask.array.core import concatenate as _concatenate |
There was a problem hiding this comment.
Please use absolute imports here:
from .creation import linspace as _linspace
from .core import concatenate as _concatenate
There was a problem hiding this comment.
Ok. Would it just be better to just import dask.array and use these functions from there?
There was a problem hiding this comment.
We use absolute imports consistently throughout. I would prefer you continue that convention here.
There was a problem hiding this comment.
Sorry, was trying to ask a different question, which maybe is being answered. Would it better to use import dask.array as da and just use da for these or is it better to import these specific functions and use them instead?
There was a problem hiding this comment.
Better to import the specific functions using absolute imports.
There was a problem hiding this comment.
Thanks for the clarification. Addresed.
|
|
||
|
|
||
| @pytest.mark.parametrize("n", list(range(1, 10))) | ||
| @pytest.mark.parametrize("d", [1.0, 0.5, 2 * np.pi]) |
There was a problem hiding this comment.
- Do we really need to check this many
n? - Slight preference to move these loops inside the test function. Up to you though.
There was a problem hiding this comment.
Maybe not, was just trying to be thorough. Certainly would want to check an even, and an odd at least. FWIW this takes ~0.1s to run as it is.
As for parametrize, this is something that @mrocklin has encouraged me to do in the past. Am kind of ok with it as it makes it easier to tell what combination failed. That said, am not really picky about this.
There was a problem hiding this comment.
I like parametrize, but only for things that are big changes (e.g. the tasks/disk parameters in the shuffle tests), but don't for things where we end up with 100s of tests for each single test function. My issue with them is mostly the increase in printed output by py.test, which isn't that big of a deal. In this case I could go either way, up to you.
There was a problem hiding this comment.
I tried handling the AssertionError in the test so we could print the parameters, but it did not go well for some reason.
Have cut down the range to 5 values. Hopefully that cuts down visual noise a little bit. Should also provide sufficient coverage. Would that work for you?
Simply leverage `linspace` to construct an appropriate range of values. Then do a little math and rearrangement to get an equivalent result to what `fftfreq` would give except for Dask Arrays.
jakirkham
force-pushed
the
add_fftfreq
branch
4 times, most recently
from
8ef5964 to
8a09672
Compare
Just provide some simple comparisons between the NumPy and Dask Array implementations of `fftfreq` to make sure that the Dask Array version behaves the same.
|
Addressed concerns raised and looks like it is passing. Please let me know if there is anything else. |
|
Thanks! |
3 participants
Fixes #2319
Adds an implementation of
fftfrequsing Dask Arrays to thefftmodule. Tests it to make sure it has the same behavior as the NumPy function over a range of different parameters.cc @jcrist