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 Copulas #130

Merged
merged 50 commits into from
Jan 31, 2022
Merged

WDRT Copulas #130

merged 50 commits into from
Jan 31, 2022

Conversation

ssolson
Copy link
Contributor

@ssolson ssolson commented Aug 11, 2021

Added Copula computations from WDRT:

  • Gaussian
  • Gumbel
  • Clayton
  • Rosenblatt
  • nonparametric Gaussian
  • nonparametric Gumbel
  • nonparametric Clayton
  • Bivaritate KDE
  • Bivaritate KDE (log)

Standard Copulas

image

NonParametric Copulas

image

KDE Plots

image

Code to Code verification

All values match the WDRT output. Plots below show Gaussian and Gumbel matching values between MHKiT and WDRT. Can view all matching plots in environmental_copulas.ipynb.

image

@ssolson ssolson changed the title Working version of Gaussian Copula Copulas: Gaussian, Gumbel Aug 17, 2021
@ssolson ssolson changed the title Copulas: Gaussian, Gumbel WDRT Copulas: Gaussian, Gumbel Aug 17, 2021
@ssolson ssolson changed the title WDRT Copulas: Gaussian, Gumbel WDRT Copulas Aug 17, 2021
@ssolson ssolson self-assigned this Sep 15, 2021
@ssolson ssolson marked this pull request as ready for review October 11, 2021 15:43
@rpauly18 rpauly18 self-requested a review December 6, 2021 16:10
@cmichelenstrofer
Copy link
Contributor

cmichelenstrofer commented Dec 7, 2021

@ssolson I did a quick review, looks good! Here are some minor comments:

environmental_contours.py

  • no need to load: pandas, _product, _signal, or _fsolve. Delete lines 4,5,6,10
  • PCA_contour function does not use sea_state_duration, or return_period. Remove these inputs.
  • In the docstrings of environmental_contours avoid calling it copula. Use contour instead, e.g. "copula method" to "contour method", and "copula component" to "component"
  • Consider renaming this file to _environmental_contours.py with the leading underscore.

resource.py

  • no need to load: KDEUnivariate, skPCA, mean_squared_error, pyplot, optimize, or stats. Delete lines 2,3,4,7,8,9
  • in line 1 also import PCA_contour
  • Consider adding comment explaining these two functions are imported from another file but belong to this module.

__init__.py

  • No need to import environmental_contours as a submodule. The relevant functions are imported into resource.py

@cmichelenstrofer
Copy link
Contributor

@ssolson I'm trying to re-think what is the best option for importing environmental_contours function into wave.resource module. These are my thoughts, I think any option will work, up to you.

  • Option 1: As is. Downside: not sure the auto-documentation will work as desired however.
  • Option 2: move the two non-hidden functions (environmental_contours, PCA_contour) directly to resource.py and only leave the hidden functions in environmental_contours.py. Downside: these two functions are still quite large compared to other functions in resource.py, and we would be splitting the functionality into two files.
  • Option 3: make the wave.resource sub-module into a folder with its own __init__.py file which would look something like:
from resource import *
from mhkit.wave.resource.environmental_contours import environmental_contours, PCA_contour

downside: deeper folder structure.

@rpauly18
Copy link
Contributor

rpauly18 commented Dec 8, 2021

@cmichelenstrofer I think how the imports are being handled is appropriate. The docs should work similarly to how we handle many of the tidal module functions

@ssolson
Copy link
Contributor Author

ssolson commented Dec 8, 2021

Agree I am finishing this up now. I went and made an example to quickly show that we curate this ourselves so in the rst file we put:

Resource
""""""""""""
The resource submodule contains functions compute wave energy spectra 
and metrics.

The following functions can be used to compute wave energy spectra:

.. autosummary::
   :nosignatures:

   ~mhkit.wave.resource.elevation_spectrum
   ~mhkit.wave.resource.pierson_moskowitz_spectrum
   ~mhkit.wave.resource.bretschneider_spectrum
   ~mhkit.wave.resource.jonswap_spectrum
   ~mhkit.wave.resource.environmental_contours

And we get:

image

@ssolson
Copy link
Contributor Author

ssolson commented Dec 8, 2021

Carlos I left the PCA method out of resource because I see it as more focused. My thought right now is that environmental_contours is available as a general purpose method which you can call from resource. You can also call environmental contours from the specific env contours module as well as the PCA method which is more focused specifically to contour methods. This is also why I chose not to _ the module itself.

Am I just complicating things and do you guys think we should just interact with everything through the resource module?

@cmichelenstrofer
Copy link
Contributor

@ssolson the only reason someone would use the environmental_contour module is for only one function that is not found elsewhere (PCA). If there were more functions maybe it would make sense, but as is my preference is to have this module hidden and users access the functions through the wave.resource module.

Also, is the output of PCA any different than calling the main environmental_contour function with the PCA method selected? I'm not convinced we need both.

But these are minor points and either option would work, so your call.

@rpauly18
Copy link
Contributor

rpauly18 commented Dec 8, 2021

My opinion is that unless we foresee more functions being developed in the environmental_contours module, everything should just be called through resource. I do like that the physical code is housed elsewhere to not overwhelm the resource.py file. That being said, if you feel strongly about having environmental_contours being separate, I am not dead set against it.

@ssolson
Copy link
Contributor Author

ssolson commented Dec 8, 2021

I will move them over.

My overall view is that the PCA is more streamlined from documentation to execution but it of course returns the same results as it should. My goal is that I ultimately believe a user will want to modify and work with these functions and having the PCA separate moves users over to the module they would want to be working in more as they look to advance their research into contour methods. But ultimately its nbd and users will find their way there naturally.

@cmichelenstrofer
Copy link
Contributor

@rpauly18 are you still reviewing this or can it be merged?

@rpauly18
Copy link
Contributor

@cmichelenstrofer I do have an open question abut a variable name (see above), but then it can be merged.

@cmichelenstrofer
Copy link
Contributor

@ssolson Can you address the minor points still open? I think this is very close to ready to merge. E.g. see @rpauly's comment and my comment about unnecessary imports and improving docstrings.

@ssolson
Copy link
Contributor Author

ssolson commented Dec 14, 2021

I don't see any questions about a variable name from @rpauly18. Could you point me to your question, please?

Component 1 data
x2: array
Component 2 data
sea_state_duration : int or float
Copy link
Contributor

Choose a reason for hiding this comment

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

@ssolson Is "sea_state_duration" the standard name for this variable? Since it looks like it is a sample rate based on the description, would something else be more suitable?

Copy link
Contributor

Choose a reason for hiding this comment

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

@ssolson here is my question.

@ssolson ssolson merged commit 6e4bff0 into MHKiT-Software:master Jan 31, 2022
@ssolson ssolson deleted the wdrt branch January 31, 2022 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request wave module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants