Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed

Conversation

simmsa
Copy link
Contributor

@simmsa simmsa commented Apr 24, 2024

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.

ssolson and others added 30 commits October 12, 2023 10:45
Use caching by default & reduce testing time
)

* run hindcast only on change

* always run hindcast on changes to master
* allow latest versions from all requirements

* add python 3.10 and 3.11 

* only test ubuntu on pulls to dev; mac & Windows on pull to master

* use actions/upload-artifact@v3; setup-python@v3; download-artifact@v3
* automatic Hs threshold

* Revert "automatic Hs threshold"

This reverts commit 4477580.

* automatic Hs threshold

* simplify & include MATLAB example for debugging

* fix independent storms

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* Update mhkit/loads/extreme.py

Co-authored-by: Adam Keester <[email protected]>

* cleanup

* Update mhkit/tests/loads/test_loads.py

* Update mhkit/tests/loads/test_loads.py

Update threshold test value one more time

* Update extreme.py

* break out nested function, consolidate scipy imports

---------

Co-authored-by: ssolson <[email protected]>
Co-authored-by: Adam Keester <[email protected]>
Co-authored-by: akeeste <[email protected]>
* Add test for loads.extreme.global_peaks function

The loads_extreme.global_peaks function was previously missing a test.

The test uses a simple function which can be independently analysed. The results of global_peaks and the independent analysis are then compared.

* Introduce upcrossing module

Previously there was no general means of performing an upcrossing analysis. The load.extreme.global_peaks function could only calculate peaks.

The module provides some common methods but also the ability for the user to define their own function over the zero crossing points.

* Implement loads.extreme.global_peaks in terms of upcrossing module

With the recent addition of the upcrossing module, we can implement the loads.extreme.global_peaks function using it.

* minor linting, module docstring, update parameter validation

* fix upcrossing docstring

* move upcrossing module to utils

* update upcrossing import in example

* fix last upcrossing docstring and move test to test/utils

* update description in the upcrossing notebook

* update import of upcrossing into test_upcrossing

* typo in test file

* final typo fix

---------

Co-authored-by: akeeste <[email protected]>
* remove ci folder

* update README installation instructions

* remove figures folder

* remove pypirc
* add silent kwarg to request_parse_workflow and get_netcdf_variables

* describe optional use of silent in the cdip example

* fix typo

* fix netcdf typo
…are#276)

* loads/extreme convert asserts to errors

* loads/general convert asserts to errors

* loads/graphics convert asserts to errors

* power module convert asserts to errors

* loads module, convert try-except validation to errors

* utils module, convert asserts to errors

* dolfyn module, convert asserts to errors

* tidal module, convert asserts to errors

* river module, convert asserts to errors

* wave hindcast, replace asserts with errors

* wave/io module, convert asserts to errors

* wave module, convert asserts to errors

* rename reserved variable min in plot_directional_spectrum

* fix miscellaneous typos

* catch new error type

* fix test logic and parameter name

* minor review comments, fix f strings, fix messages, etc

* list correct types in error messages, f strings, etc

* standardize error messages for optional parameters

* Apply suggestions from ssolson's 2nd review
…HKiT-Software#278)

* check is samples min or max included in the contour half

* ensure requested samples are within range of contour values
* Added limits to variable_interpolation and added 3 array input capability to create_points


Co-authored-by: Browning <[email protected]>
Co-authored-by: ssolson <[email protected]>
* loads/general

* loads/extreme conversion to xarray

* test and bugfix for loads/general/bin_statistics

* fix bug in bin_statistics where std=0

* correct bin_statistics test data

* update bin_statistics test

* fix dimension name in mler_wave_amp_normalize

* formatting fixes

* update return types, add optional dimension argument

* add Series and DataArray to type check for mler_coefficients

* update argument and docstring for time_dimension

* update dimension variables to time_dimension

* rename time_dimension to frequency_dimension where required
* MHKiT-Software#277 
* MHKiT-Software#284 - Fix window check
* Test updates plus compression bugfix
* power/quality add basic formatting

* initial conversion or power/quality submodule

* add xarray tests for power.quality

* fix variable assignment in power.quality

* power.characteristics add basic formatting for xr conversion

* update error messages

* finish converting power.quality to xarray

* fix spaces/formating in docstrings

* clean up conversion of inputs from pandas to xr.dataset

* add tests for xarray

* update handling of timestamps in power.characteristics for xr

* fix length in power.quality.harmonics

* fix length in power.quality.harmonics, again

* add frequency_dimension and time_dimension arguments

* remove old imports

* remove obsolete argument from THCD tests

* type check on to_pandas

* add type and value checks for time_dimension and frequency_dimension

* make grid_freq checks f strings that return the incorrect value

* add frequency_dimension valueError

* add formal docstring to _convert_to_dataset

* add line_to_line type check

* restore old naming convention to ac_power_three_phase

* update example call to THCD

* return hard coded test answers to being recalculated
- black formatting
- GitHub actions black
- new API key added for NREL's HSDS server
- `mhkit.tests.river.test_io` broken out into `mhkit.tests.river.test_io_d3d` and `mhkit.tests.river.test_io_usgs`
- added functional tests and error handing tests to `mhkit.tests.dolfyn.test_tools`, `mhkit.tests.mooring.test_mooring`
- added error handling tests for `mhkit.tests.river.test_resource`, `mhkit.tests.wave.io.hindcast.test_wind_toolkit`, `mhkit.tests.wave.test_contours`, `mhkit.tests.loads.test_loads`
@ssolson
Copy link
Contributor

ssolson commented Apr 25, 2024

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

@simmsa
Copy link
Contributor Author

simmsa commented Apr 25, 2024

@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

eta = xr.Dataset()
for var in S.data_vars:
if phases is None:
np.random.seed(seed)
phase = xr.DataArray(
data=2 * np.pi * np.random.rand(S[var].size),
dims="Frequency",
coords={"Frequency": f},
)
else:
phase = phases[var]

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
Copy link
Contributor Author

simmsa commented Apr 25, 2024

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

@simmsa
Copy link
Contributor Author

simmsa commented Apr 26, 2024

@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.

@simmsa simmsa marked this pull request as ready for review April 26, 2024 14:40
ssolson added 5 commits April 26, 2024 20:38
This PR addresses 2 issues with the hindcast tests:

* On PRs tests were only being run on changes to hindcast test files.
  - Fixed to run on changes to hindcast test and module files.
* The prepare cache job was failing due to 6 hour job limit.
  - Fixed by breaking out each hindcast cache test into its own job.

* Additionally this PR updates action packages to the latest version where possible.
conda-incubator/setup-miniconda@v3 to install conda.
- Uses python version correctly
- Installs MHKiT dependencies in conda build 
- Use coverage actions instead of coverage CLI 
- Uses an updated coverage version with lcov support
- Adds an `environment.yaml` file for conda environment install 
- Special case for MacOS-latest (macos 14)  and Py 3.8
- CI test job `set-os` logic modified to now run all OS on push to `develop`
- `hindcast-calls` job now uses the environment.yaml file & forces the correct coverage version as added in MHKiT-Software#317 
- Wind cache waits on wave cache completion
* set flag name

* use master coveralls-app action

* v0.8.0
@ssolson ssolson changed the base branch from develop to main May 8, 2024 13:27
@ssolson ssolson deleted the branch MHKiT-Software:develop May 8, 2024 14:14
@ssolson ssolson closed this May 8, 2024
@ssolson ssolson reopened this May 8, 2024
@ssolson
Copy link
Contributor

ssolson commented May 8, 2024

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.

simmsa added 10 commits May 8, 2024 10:31
* `test_surface_elevation_auto_vs_ifft`:
    * Verifies that an input spectrum with a zero frequency uses the
      `ifft` method
* `test_surface_elevation_auto_vs_sum_of_sines`:
    * Verifies that an input spectrum without a zero frequency uses the
      `sum_of_sines` method
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.
User should be warned that the computation method is going to be sum of
sines if the input spectrum does not have a zero frequency index.
Remove the auto option from the surface elevation method.
If a user inputs a spectrum without a zero frequency and the method is
ifft, warn the user and use the sum_of_sines method.
@simmsa simmsa force-pushed the surface-elevation-auto-method branch from 8db6444 to af92811 Compare May 8, 2024 16:35
@ssolson
Copy link
Contributor

ssolson commented Jun 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants