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

WDRT parity #127

Merged
merged 5 commits into from
Jul 1, 2024
Merged

WDRT parity #127

merged 5 commits into from
Jul 1, 2024

Conversation

hivanov-nrel
Copy link
Contributor

@hivanov-nrel hivanov-nrel commented Apr 30, 2024

This PR is to address MATLAB parity for:

Below are the list of functions & tests to be added:

extreme.py:

  • ste_peaks
  • block_maxima
  • ste_block_maxima_gev
  • ste_block_maxima_gumbel
  • short_term_extreme
  • full_seastate_long_term_extreme

peaks.py:

  • global_peaks
  • number_of_short_term_peaks
  • peaks_distribution_weibull
  • peaks_distribution_weibull_tail_fit
  • peaks_distribution_peaks_over_threshold

contours.py:

  • samples_full_seastate
  • samples_contour

Testing:

  • add tests
  • tests are passing locally

Below are list of examples to be added:

  • extreme_response_contour_example
  • environmental_contours_example (update outdated function call)
  • short_term_extreme_example
  • extreme_response_full_sea_state_example

Below are a list of functions/notebooks not incorporated:

  • sample.py - return_year_value -> io is a bit complicated
  • environmental_contours_example -> need to add additional copula methods besides PCA
  • automatic_hs_threshold -> part of version 0.8.0 which is not yet supported on matlab side

@hivanov-nrel hivanov-nrel marked this pull request as ready for review June 12, 2024 23:14
@hivanov-nrel
Copy link
Contributor Author

Hi @simmsa @rpauly18 I think this is ready for review. The code/tests that I worked are passing locally.

The only thing that I am unsure about is that comparing the notebooks to python, there are some instances where the values differ. For instance, the extreme design wave height seems suspiciously low. However, the tests are passing using the same data as python so i'm a bit confused on where the difference can be coming from...

Copy link
Contributor

@simmsa simmsa left a comment

Choose a reason for hiding this comment

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

@hivanov-nrel, overall this is a great improvement! There are a few failing tests due to file path issues and some files that were not included. I addressed these with specific comments in this PR.

I was able to reproduce the odd output in the extreme_response_contour_example Livescript. This issue is due to the gamma parameter not being correctly mapped to MHKiT-Python. The related issue is here #132. I plan to add a PR from the develop branch that fixes this asap. With a fixed jonswap_spectrum function we get a response that is closer to MHKiT-Python.
Screenshot 2024-06-26 at 2 48 18 PM.

In extreme_response_contour_example, extreme_response_full_sea_state_example, and short_term_extremes_example I am getting results that are different that python. This is most likely due to randomness introduced not specifically setting a random seed in the surface_elevation function. At a minimum we should probably set a seed on the MATLAB side. Looking into the python side, the default seed is none (https://github.com/MHKiT-Software/MHKiT-Python/blob/83a9d6d77841bd9da6f0d494c1df9f4bb43d58b0/mhkit/wave/resource.py#L363) and is random. We should push for setting a seed on the Python side when we have time, as it would be great if the MATLAB and Python notebooks had the same results.

mhkit/tests/Loads_TestExtreme.m Outdated Show resolved Hide resolved
mhkit/tests/Wave_TestContours.m Outdated Show resolved Hide resolved
mhkit/tests/Wave_TestContours.m Outdated Show resolved Hide resolved
mhkit/tests/Loads_TestExtreme.m Outdated Show resolved Hide resolved
Copy link
Contributor

@simmsa simmsa left a comment

Choose a reason for hiding this comment

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

@hivanov-nrel thank you for fixing all the requested items. All of the tests are passing locally. I am going to go ahead and merge this into develop. Thank you for getting this done!

image

@simmsa simmsa merged commit f49fbf0 into MHKiT-Software:develop Jul 1, 2024
0 of 7 checks passed
@simmsa simmsa mentioned this pull request Jul 8, 2024
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.

2 participants