In `wave.resource.surface_elevation` use `sum_of_sines` method if input spectrum does not have a zero frequency

[simmsa]

trafficstars

Add auto method to wave.resource.surface_elevation to address #308.

The auto method chooses the most computationally efficient surface elevation computation method based on the input spectrum. auto uses the sum_of_sines method is a zero frequency is not defined in the input spectrum index and uses the ifft for all other cases.

[akeeste]

Thanks @simmsa. The other xarray PR is merged so we can address this quick before our release. I definitely like addressing the zero-frequency behavior in some way without an error. I think we should either:

• default to the auto method, but not throw a warning. I think an automatic method already implies we're choosing for the user, though the warning does indicate which method is ultimately chosen.
• or keep ifft as the default method, and revert to sum_of_sines with a warning if there is no zero frequency

I'm leaning towards the latter since there are less options for the user to choose from. Thoughts?

[simmsa]

@akeeste, the latter is a great suggestion and simplifies the design. The latest changes remove auto and set ifft as the default method. If the method is ifft and the user inputs a spectrum with a zero frequency we warn the user and set the method to sum_of_sines.

[ssolson]

Please do not merge this until after we get #307 into master.

[simmsa]

@akeeste 9b3bc3b changes the strategy for accessing the first frequency index value. Does that method seem more reliable for accessing the first value of the frequency index?

I am seeing some interesting test failures in MHKiT-MATLAB that are within the surface elevation function that I am hoping are not related to this change:

Test:

       function test_surface_elevation_seed(testCase)

           Obj.f = 0.0:0.01/(2*pi):3.5/(2*pi);
           Obj.Tp = 8;
           Obj.Hs = 2.5;
           df = 0.01/(2*pi);
           Trep = 1/df;
           Obj.t = 0:0.05:Trep;

           S = jonswap_spectrum(Obj.f, Obj.Tp, Obj.Hs);
           seednum = 123;
           eta0 = surface_elevation(S, Obj.t);
           eta1 = surface_elevation(S, Obj.t,"seed",seednum);
           assertEqual(testCase,eta0, eta1);
       end

Failure:

================================================================================
Error occurred in Wave_TestResourceSpectrum/test_surface_elevation_seed and it did not run to completion.
    ---------
    Error ID:
    ---------
    'MATLAB:Python:PyException'
    --------------
    Error Details:
    --------------
    Error using dataarray>_check_coords_dims
    Python Error: ValueError: coordinate Frequency has dimensions ('dim_0',), but these are not a subset of the DataArray dimensions ('Frequency',)
    
    Error in dataarray>_infer_coords_and_dims (line 193)
    
    Error in dataarray>__init__ (line 454)
    
    Error in resource>surface_elevation (line 364)
    
    Error in Wave_TestResourceSpectrum/test_surface_elevation_seed (line 43)
                eta0 = surface_elevation(S, Obj.t);
===========================================================

The error appears to be originating from here https://github.com/MHKiT-Software/MHKiT-Python/blob/19b4df0c92a2b80575b7f97f62944f1a20273090/mhkit/wave/resource.py#L360-L370

It seems like xarray is really picky about the names of the dimensions. In the MATLAB surface_elevation function we pass a dataframe into the wave.resource.surface_elevation function. Any thoughts? Getting this far I don't think this is an issue related to this PR.

[simmsa]

@ssolson, sorry for the poor timing of this PR. This does not need to be included in the latest release. Thank you!

[simmsa]

@akeeste this is good to go now! The test failure in MHKiT-MATLAB is unrelated to this change. #313 adds a fix for the dim_0 error.

[ssolson]

Hey @simmsa apologies for messing this PR up. This should be easier going forward but there were some inconsistencies in how PR merged strategies were applied by the final merge author which made this messier. I perhaps should have just accepted the mess but instead decided to squash into master. My preferred strategy going forward is to squash PRs to develop and merge by ort into master.

To ensure master and develop are even I did a force rebase on origin develop.

That said can you either rebase or resubmit this PR? The official approach is to:

git checkout develop
git fetch origin
git reset --hard origin/develop

git checkout feature-branch
git rebase develop
git push user-fork feature-branch --force

That said rebasing may be extremely painful and it might be easier to just start from the new develop and remake the changes. Apologies for the trouble on this.

[ssolson]

@simmsa when you get a chance could you look to resolve the merge conflicts to we can close this PR?