I made spectraldiff capable of handling multidimensional data. The 4 …#186
I made spectraldiff capable of handling multidimensional data. The 4 …#186mariaprot wants to merge 1 commit intoflorisvb:masterfrom
Conversation
…test cases for multidimensionality pass now, but when we test it in the multidimensionality_demo it is very sensitive. Also, we made sure that spectraldiff is now in the multidimensionality_demo.
|
|
||
| from pynumdiff.utils import utility | ||
|
|
||
| #maria spectral diff below |
There was a problem hiding this comment.
Can you remove this line and any others that aren't essential?
| #maria spectral diff below | ||
|
|
||
| def spectraldiff(x, dt, params=None, options=None, high_freq_cutoff=None, even_extension=True, pad_to_zero_dxdt=True): | ||
| def spectraldiff(x, dt, axis=0, params=None, options=None, high_freq_cutoff=None, |
There was a problem hiding this comment.
axis actually shouldn't be in the third position here, due to a backwards compatibility quirk. The method signature/header is laid out so that params given positionally in the third position can be parsed. This is just in case old code from @florisvb's lab calls functions according to the old style (pre my extension to use keyword arguments). This won't matter once I eventually take care of #183, but does in the mean time. Sorry, it's confusing. Maybe move axis to the end of the list? That would match finitediff and savgoldiff.
| - **dxdt_hat** (np.array) -- estimated derivative of x | ||
| """ | ||
| if params is not None: # Warning to support old interface for a while. Remove these lines along with params in a future release. | ||
| if params is not None: |
There was a problem hiding this comment.
Can my old comment come back? Or is it kind of distracting and better without?
| # make derivative go to zero at ends (optional) | ||
| if pad_to_zero_dxdt: | ||
| padding = 100 | ||
| pre = getattr(x, 'values', x)[0]*np.ones(padding) # getattr to use .values if x is a pandas Series |
There was a problem hiding this comment.
mmm. I am seeing that cleaning up this confusing way of handling a series vs an array by simply calling asarray earlier may be better.
| pre = np.repeat(first, padding, axis=0) | ||
| post = np.repeat(last, padding, axis=0) | ||
|
|
||
| xpad = np.concatenate((pre, x0, post), axis=0) # i think hstack won't work with the correct axis |
There was a problem hiding this comment.
I believe you're right that hstack and vstack just take vectors. Look in the documentation to find out for sure and update the comment to be similarly certain.
| X = np.fft.fft(x) | ||
| x_hat = np.real(np.fft.ifft(filt * X)) | ||
| x_hat = x_hat[padding:L+padding] | ||
| # Smoothed signal |
There was a problem hiding this comment.
A couple extra spaces ended up at the start of this line. Can you put the comment at the same indentation as the code? It's intended to be a little header.
| :param np.array[float] x: data to differentiate | ||
| :param float or array[float] dt_or_t: This function supports variable step size. This parameter is either the constant | ||
| :math:`\\Delta t` if given as a single float, or data locations if given as an array of same length as :code:`x`. | ||
| :math:`\\Delta t` if given as a single float, or data locations if given as an array of same length as :code:`x`. |
There was a problem hiding this comment.
I think this line actually does need to be more indented, because it's a continuation of the description for the parameter on the line above. I don't think Sphinx will render it properly if it's not. You can pip install sphinx and cd to the docs folder and call make html to generate the documentation. A bunch of .html files will then live in the docs/build/html folder, and you can open them and navigate around with a web browser. When the code is merged, readthedocs essentially does that same build on their server and then deploys the resulting files so they're rendered when we visit pynumdiff.readthedocs.io.
| X = np.fft.fft(x0, axis=0) | ||
|
|
||
| # Derivative = 90 deg phase shift | ||
| omega = 2*np.pi/(dt*N) # factor of 2pi/T turns wavenumbers into frequencies in radians/s |
There was a problem hiding this comment.
I like this comment. Can it come back?
| # Filter to zero out higher wavenumbers | ||
| discrete_cutoff = int(high_freq_cutoff*N/2) # Nyquist is at N/2 location, and we're cutting off as a fraction of that | ||
| discrete_cutoff = int(high_freq_cutoff * N / 2) # Nyquist is at N/2 location, and we're cutting off as a fraction of that | ||
| filt = np.ones_like(k, dtype=float) |
There was a problem hiding this comment.
I think there might be something fishy going on with this filt. k is a vector, so ones_like will return a vector too. But then it's redefined on the next line anyway to be ones(k.shape), and I believe the default data type is floats, so the first declaration isn't doing anything. Then we properly set 0s. Then reshaping I think is to make filt * X work, looks like. Maybe add a comment to that effect.
2 participants
…test cases for multidimensionality pass now, but when we test it in the multidimensionality_demo it is very sensitive. Also, we made sure that spectraldiff is now in the multidimensionality_demo.