Skip to content

Conversation

@cmichelenstrofer
Copy link
Contributor

@cmichelenstrofer cmichelenstrofer commented Nov 9, 2021

Fixes #140. Fixes #142.

@cmichelenstrofer cmichelenstrofer changed the title Fix wave elevation function. Fixes #140. Fixes #142 Fix wave elevation function. Nov 9, 2021
@cmichelenstrofer cmichelenstrofer added bug Something isn't working Clean Up Improve code consistency and readability wave module labels Nov 9, 2021
@cmichelenstrofer
Copy link
Contributor Author

cmichelenstrofer commented Nov 9, 2021

@ssolson and @rpauly18 could you review this?

  • Bug fix:
    • not truncating the spectrum anymore (was being truncated based on time value, but the pd.Series was indexed by frequency).
  • Speedup
    • moved calculation of B outside loop
    • replaced list comprehension + itertools with np.outer function
    • avoided creating a complex array to then take the real part only, instead create the real part directly.
  • Functionality:
    • changed default seed to None. The default behavior should be creating a random phase. A user can specify a seed for reproducibility. Should not be the other way around (where we were hardcoding a phase as the default behavior).
  • Tests:
    • There were two tests that did nothing. They would always pass. Their intention were to check that the default behavior and specifying the seed in the function signature gave the same results. But they always will even if the function is broken. I commented these out but think they should be deleted.
    • There were other tests that relied on the seed being fixed. Added a seed to those tests.
  • Examples:
    • I couldn't find an existing notebook that uses the surface_elevation function. If there isn't one, maybe we should add it to an existing example.
  • Reference
    • We need to find a good reference for this function (converting frequency domain to time domain)

@rpauly18
Copy link
Contributor

@cmichelenstrofer happy to review this. Are you still working on the fix for why the tests are failing, or would you like for me to look into this?

@cmichelenstrofer
Copy link
Contributor Author

@rpauly18 I just fixed the tests. You can go ahead and review it. Thanks!

@rpauly18 rpauly18 self-requested a review November 19, 2021 18:58
Copy link
Contributor

@rpauly18 rpauly18 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Are you still working on more changes, or is this ready to be converted from a draft?

@cmichelenstrofer cmichelenstrofer marked this pull request as ready for review November 22, 2021 16:20
@cmichelenstrofer
Copy link
Contributor Author

@rpauly18 just changed it from 'draft' to 'ready to review'. I deleted the two obsolete tests that I had commented out.

@cmichelenstrofer
Copy link
Contributor Author

cmichelenstrofer commented Nov 22, 2021

Something wrong with the tests. They were passing before and nothing changed other than deleting comments. Seems the issue is not with this PR but with accessing some data at NREL needed for the tests.

requests.exceptions.RetryError: HTTPSConnectionPool(host='developer.nrel.gov', port=443): 
Max retries exceeded with url: /api/hsds/datasets/d-9db3e298-adf30787-6aa4-5dda8d-a672ee?domain=%2Fnrel%2FUS_wave%2Fvirtual_buoy%2FWest_Coast%2FWest_Coast_virtual_buoy_1995.h5&api_key=3K3JQbjZmWctY0xmIfSYvYgtIcM3CN0cb1Y2w9bf 
(Caused by ResponseError('too many 503 error responses'))

@rpauly18
Copy link
Contributor

@cmichelenstrofer random failures happen sometimes because of the WPTO hindcast server being overloaded with request. I restarted the tests. If they fail again, I will investigate further.

@cmichelenstrofer
Copy link
Contributor Author

@rpauly18 can this be merged?

@rpauly18 rpauly18 merged commit 368b3ff into MHKiT-Software:master Dec 1, 2021
@cmichelenstrofer cmichelenstrofer deleted the Wave-elevation-bug-fix branch December 17, 2021 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Clean Up Improve code consistency and readability wave module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Time array not used correctly in wave.resource.surface_elevation wave elevation function is very slow

2 participants