wave.resource.surface_elevation does not gracefully handle input wave spectra where the zero frequency is not defined

[simmsa]

Describe the bug:

In #250 the method option was added to the wave.resource.surface_elevation function. method defaults to using the iftt method for calculating the surface elevation. To use iftt method the input spectrum frequency must have an zero frequency defined:

MHKiT-Python/mhkit/wave/resource.py

Lines 276 to 280 in b8e832a

	if method == "ifft":
	if not S.index.values[0] == 0:
	raise ValueError(
	f"ifft method must have zero frequency defined. Lowest frequency is: {S.index.values[0]}"
	)

Alternatively, the sum_of_sines method can handle frequency indexes without a zero frequency defined. If the input wave spectrum does not have a zero frequency defined should we automatically use the sum_of_sines method instead of producing an error? Right now this option is configurable by the user, but it may make sense to automatically select the correct method based on the input data.

To Reproduce:

import mhkit.wave as wave
import numpy as np

Hs = 1.5
Tp = 16
frequency_indexes = np.linspace(1 / 30, 1 / 2, num=32)

S = wave.resource.pierson_moskowitz_spectrum(frequency_indexes, Tp, Hs)

duration_seconds = 60 * 10
delta_time = 0.1
time_index = np.arange(0, duration_seconds, delta_time)
wave_elevation_time_series = wave.resource.surface_elevation(S, time_index)

AssertionError: ifft method must have zero frequency defined

Expected behavior:

The wave.resource.surface_elevation function should gracefully handle an input spectrum that does have include a zero frequency defined in the input spectrum. One option would be to change the default method to auto and have the surface_elevation function choose either iftt or sum_of_sines based on the input spectrum.

I can see that we are actively working on this code in #302. Once that is complete we should discuss if the auto method would be an acceptable solution to this issue.

Possible Implementation:

diff --git a/mhkit/wave/resource.py b/mhkit/wave/resource.py
index b1c3a2d..3a7bbb1 100644
--- a/mhkit/wave/resource.py
+++ b/mhkit/wave/resource.py
@@ -273,11 +273,11 @@ def surface_elevation(
         if not (method == "ifft" or method == "sum_of_sines"):
             raise ValueError(f"Method must be 'ifft' or 'sum_of_sines'. Got: {method}")
 
-    if method == "ifft":
+    if method == "auto" and frequency_bins is None:
         if not S.index.values[0] == 0:
-            raise ValueError(
-                f"ifft method must have zero frequency defined. Lowest frequency is: {S.index.values[0]}"
-            )
+            method = "sum_of_sines"
+        else:
+            method = "ifft"
 
     f = pd.Series(S.index)
     f.index = f

Desktop (please complete the following information):

• OS: MacOS Ventura 13.6.6
• MHKiT Version: 0.7.0

Additional context:

@ckberinger reported Issue #125 in MHKiT-MATLAB that details a use case that highlights this error. At this time the method option is not available to the MATLAB user, (There is a working PR to add this feature).

[akeeste]

I support reverting to sum of sines if the zero frequency is not available. We could throw a warning instead so the user is aware of the method being used

[simmsa]

@akeeste, good idea! I added a warning and a test in #309.

[simmsa]

This issue was fixed in #309.