MHKiT-Software / MHKiT-Python

MHKiT-Python provides the marine renewable energy (MRE) community tools for data processing, visualization, quality control, resource assessment, and device performance.
https://mhkit-software.github.io/MHKiT/
BSD 3-Clause "New" or "Revised" License
47 stars 45 forks source link

Bug: Dolfyn `cpsd_quasisync_1D` cannot handle np.array #284

Closed ssolson closed 5 months ago

ssolson commented 5 months ago

@jmcvey3 can you let me know if I am approaching this incorrectly?

Description:

The docstring of cpsd_quasisync_1D claims that I can pass a custom np.array window.

https://github.com/MHKiT-Software/MHKiT-Python/blob/2ccc286a65685e5e2f5ab68a467209917f0161d9/mhkit/dolfyn/tools/fft.py#L81-L162

However _getwindow has not implemented that logic:

https://github.com/MHKiT-Software/MHKiT-Python/blob/2ccc286a65685e5e2f5ab68a467209917f0161d9/mhkit/dolfyn/tools/fft.py#L34-L41

Test Case Issue:

The following test will fail when I pass a custom window.

    def test_cpsd_quasisync_1D(self):
        fs = 1000  # Sample rate
        nfft = 512  # Number of points in the fft

        # Test with signals of same length
        a = np.random.normal(0, 1, 1000)
        b = np.random.normal(0, 1, 1000)

        # Test with a custom window
        custom_window = np.hamming(nfft)
        cpsd = tools.fft.cpsd_quasisync_1D(a, b, nfft, fs, window=custom_window)
        self.assertEqual(cpsd.shape, (nfft // 2,))

Proposed solution

I think _getwindow simply needs a check to make sure the passed window is within bounds and then return the custom window

def _getwindow(window, nfft):
    # Check if the window is a numpy array
    if isinstance(window, np.ndarray):
        # Check if the length of the custom window matches nfft
        if len(window) != nfft:
            raise ValueError("Custom window length must be equal to nfft")
        return window
    # Existing conditions for string-specified windows
    elif window == "hann":
        window = np.hanning(nfft)
    elif window == "hamm":
        window = np.hamming(nfft)
    elif window is None or window == 1:
        window = np.ones(nfft)
    return window
jmcvey3 commented 5 months ago

This is not something I've ever tested.

Through the dolfyn TimeBinner or VelBinner classes it should be able to handle if you want to enter a different # of FFT to calculate CSDs. Dolfyn will then check the length of the two signals - if they aren't the same length, it'll use cpsd_quasisync and if they are, it'll use cpsd.

jmcvey3 commented 5 months ago

Looks like cpsd_quasisync does not return the half spectra; it only returns the full. cpsd will return the half spectra if you give it the additional argument step=nfft//2

jmcvey3 commented 5 months ago

cpsd_1D(a, b, nfft, fs, window=custom_window, step=nfft//2) will get you what you want.

jmcvey3 commented 5 months ago

Added the fix for the window argument here: https://github.com/MHKiT-Software/MHKiT-Python/pull/280/commits/98e1a144a9602295d2bbbab4bdb6d6fa54604ecf