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
50 stars 45 forks source link

Time array not used correctly in wave.resource.surface_elevation #142

Closed cmichelenstrofer closed 2 years ago

cmichelenstrofer commented 2 years ago

Currently the different spectral arrays are being truncated using the start and end time, e.g.

phase = phase[start_time:end_time]

but these Panda arrays are indexed by frequency (Hz) not time. It is not doing what it is supposed to be doing.

cmichelenstrofer commented 2 years ago

Should the spectrum be truncated at all?

ssolson commented 2 years ago

Hey Carlos this is a good discussion to have. There was a switch made early on in MHKiT for the wave module where a decsion was made to switch from time indexed to frequency indexed and I wonder if this is leftover from that. I also see that there is a comment questioning if things should be truncated at all. Similar to #139 this seems to be a non IEC formula and I cannot find a good reference for it. @kmruehl do you know where this formulation came from?

cmichelenstrofer commented 2 years ago

I'm pretty sure the correct way would be to not truncate.

cmichelenstrofer commented 2 years ago

Also, the frequency bins are no longer needed as a input. I can see that

delta_f[0] = f[1]-f[0]

was replaced with

delta_f = f.diff()

Since S is indexed by frequency we can just calculate the entire delta_f like so for all cases, so this is not needed:

"""
frequency_bins: numpy array or pandas Series (optional)
        Bin widths for frequency of S. Required for unevenly sized bins
"""
cmichelenstrofer commented 2 years ago

Also, the default value of phase should be None so that you can call the function repeatedly and create distinct realization of the sea state. I modified this, and made some changes to make the function faster, in the WDRT draft pull request (#141).

kmruehl commented 2 years ago

@cmichelenstrofer which function is this related to?

@ssolson did you get my email with references related to #139? I think the formulation should be changed and citation added.

cmichelenstrofer commented 2 years ago

@kmruehl wave.resource.surface_elevation

kmruehl commented 2 years ago

@cmichelenstrofer I agree that omega and phase should not be truncated by start_time and end_time since S is indexed by frequency, not time. I'm not sure when/why this was introduced, but it should be removed. Looking at the history of this file, it appears to have been done prior to migrating to this repository, and the inline comment has existed since that time. Also, at that time frequency_bins was not an optional input, but it is a necessary is the desired frequency bins aren't evenly spaced.

cmichelenstrofer commented 2 years ago

it is necessary if the desired frequency bins aren't evenly spaced.

@kmruehl I think you are right, delta_f = f.diff() is not adequate because the given frequencies are at the bin centers not edges. Then I think we should:

  1. Check that the frequency spacing is constant when the frequency_bins are not given.
  2. Revert to delta_f[0] = f[1]-f[0] instead of delta_f = f.diff() (scalar instead of vector) when frequency_bins are not given.
cmichelenstrofer commented 2 years ago

Note, there is a typo/bug:

if frequency_bins is not None:
        assert frequency_bins.squeeze().shape == frequency_bins.squeeze().shape,(
            'shape of frequency_bins must match shape of S')

The right hand side of the equality check should be S not frequency_bins again (which is always true).

cmichelenstrofer commented 2 years ago

Also, when would the for loop be used? when does S have more than one column?

cmichelenstrofer commented 2 years ago

@ssolson I can make these changes and the ones in #140 and do a PR. The only thing I need to know is if there are cases when S has multiple columns.

kmruehl commented 2 years ago

@cmichelenstrofer , yes there are instances when S has multiple columns