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

Include VIROCON contour methods? #201

Open
cmichelenstrofer opened this issue Nov 9, 2022 · 12 comments
Open

Include VIROCON contour methods? #201

cmichelenstrofer opened this issue Nov 9, 2022 · 12 comments
Labels
enhancement New feature or request

Comments

@cmichelenstrofer
Copy link
Contributor

I will look at possibly including the contour methods from the Virocon package.
E.g. this one. @ryancoe will reach out to them to make sure it is OK.

@cmichelenstrofer cmichelenstrofer added the enhancement New feature or request label Nov 9, 2022
@cmichelenstrofer cmichelenstrofer self-assigned this Nov 9, 2022
@ssolson
Copy link
Contributor

ssolson commented Nov 9, 2022

Sounds good to reach out. Looking at the package they have an MIT license which is permissive with respect to basically everything.

... to deal in the Software without restriction, including without limitation the rights to use, copy, modify, merge, publish,
distribute, sublicense, and/or sell copies of the Software, ...

Are you thinking of making it a dependency or copying a specific method?

@cmichelenstrofer
Copy link
Contributor Author

cmichelenstrofer commented Nov 9, 2022

Yes, but since this is a past collaborator @ryancoe wants to make sure (give them a heads up).

I was thinking of making a dependency, but I have not looked at the code in detail yet.

@ryancoe
Copy link
Contributor

ryancoe commented Nov 9, 2022

@cmichelenstrofer - can you illustrate what this would look like? My understanding is something like

from virocon import contours

def mhkit_mapping_of_contour1(args)
    return contours.contour1(args)

Is that right? And the benefit would be that we would also add documentation for each of these in MHKiT (instead of referring to ViroCon documentation)?

@cmichelenstrofer
Copy link
Contributor Author

The benefit would be to have all our contour methods we use in the same package (MHKiT) instead of across packages (MHKiT, WDRT, ViroCon, etc). Even if the actual code is in another package we will have the full list in MHKiT and have Docstrings for it. The QC module in MHKiT similarly uses other dependencies, it is just a wrapper.

@cmichelenstrofer
Copy link
Contributor Author

I haven't looked at their code but there might need to be some modification of the args so that the MHKiT args are consistent with the other methods in MHKiT (that's another potential benefit). It would be something like

from virocon

def environmental_contour(method, args...):
    ... 
    if method=="OMAE":
        virocon_args = .... # some function of the args
        contour = virocon.contours.OMAE(virocon_args)
   ...
   return

@cmichelenstrofer
Copy link
Contributor Author

@ryancoe we have one single contour function and you pass the method as one of the inputs.

@ryancoe
Copy link
Contributor

ryancoe commented Nov 9, 2022

that's helpful, thanks. If you have a moment, take a look at how virocon is structured. I think it is pretty elegant.

@cmichelenstrofer
Copy link
Contributor Author

If you have a moment, take a look at how virocon is structured. I think it is pretty elegant.

They use a class structure like we did in WDRT. It is well organized and they have several methods we do not. It is a nice package.

@ssolson and @akeeste something we have done in WecOptTool is try to use as much as possible existing packages, so that means we have had to make contributions to other packages to get what we need. But then the maintenance is not on us and there is no duplication of capabilities. If in the future you want to move in that direction for MHKiT,

  • virocon could have all the environmental contours (you could contribute Aubrey's PCA and anything else it might be missing).
  • wavespectra here can be used for creating wave spectra (e.g. JONSWAP), gathering buoy data (e.g. NDBC) etc.
  • etc.

For WecOptTool we have been using wavespectra and helping them improve it. Just a suggestion for longer-term development.

@ryancoe for our immediate needs I think going ahead and wrapping those functions like we discussed above is the best option.

@ryancoe
Copy link
Contributor

ryancoe commented Nov 9, 2022

The aspect of the virocon structure that I like is how they break up the statistical model fitting and contour construction into different steps (see illustration here): https://virocon.readthedocs.io/en/latest/workflow.html

@cmichelenstrofer
Copy link
Contributor Author

cmichelenstrofer commented Nov 9, 2022

I was just looking at that. It's pretty cool! For the same model (e.g. OMAE2020) you can choose different methods: IFORM, ISORM, Direct, Highest Density. I really need to understand this stuff better tbh.

@cmichelenstrofer
Copy link
Contributor Author

cmichelenstrofer commented Nov 9, 2022

@ryancoe That separation makes it different enough from MHKiT. It would need more thought on how to incorporate them in here. I say let's table this then. @Graham-EGI For the DLC we can simply use the one method from virocon that we want directly. @ssolson @akeeste up to you if you want to incorporate these methods in the future and whether to keep this issue open.

@cmichelenstrofer cmichelenstrofer removed their assignment Nov 9, 2022
@ssolson
Copy link
Contributor

ssolson commented Nov 9, 2022

@ssolson and @akeeste something we have done in WecOptTool is try to use as much as possible existing packages, so that means we have had to make contributions to other packages to get what we need. But then the maintenance is not on us and there is no duplication of capabilities. If in the future you want to move in that direction for MHKiT,

  • virocon could have all the environmental contours (you could contribute Aubrey's PCA and anything else it might be missing).
  • wavespectra here can be used for creating wave spectra (e.g. JONSWAP), gathering buoy data (e.g. NDBC) etc.
  • etc.

For WecOptTool we have been using wavespectra and helping them improve it. Just a suggestion for longer-term development.

@ryancoe for our immediate needs I think going ahead and wrapping those functions like we discussed above is the best option.

myFile (2)

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

No branches or pull requests

3 participants