Skip to content

Dev: adding ampSpectraForWS function #907

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.

Sign up for GitHub

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

Jump to bottom
Merged
akeeste merged 10 commits into from
Aug 29, 2022
Merged

Dev: adding ampSpectraForWS function #907

akeeste merged 10 commits into from
Aug 29, 2022

Conversation

@dforbush2
Copy link
Contributor

@dforbush2 dforbush2 commented Jul 7, 2022

This is related to issue #873 . While we recommend spectrumImport instead of elevationImport as a more robust solution method for user-defined wave time series, we previously have not provided a tool to convert the elevationImport.mat --> spectrumImport.mat. The addition of this single function fixes that.

@kmruehl kmruehl requested a review from akeeste July 13, 2022 14:37
@kmruehl kmruehl added the Wave Class Wave Classs (waveClass.m) label Jul 13, 2022
@akeeste
Copy link
Contributor

akeeste commented Jul 21, 2022

@dforbush2 I tested the function with a sample irregular wave. It works well and gives very similar results to MHKiT. I don't have any issue with the function, but prefer to encourage users to use MHKiT for this purpose. MHKiT already has tests and documentation built in as well. If users have MHKiT-MATLAB installed, it would be straightforward to use their elevation_spectrum function.

pr907 amp spectrum

akeeste
akeeste requested changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@akeeste akeeste left a comment

Choose a reason for hiding this comment

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

@dforbush2

Per my previous comments, I made a PR into your dev branch with a few updates, including:

  • remove WEC-Sim ampSpectraForWS funtion
  • add to your documentation update pointing users to MHKiT
  • remove changes to source/functions/simulink/model/nonLinearBuoyancy.m which i believe is meant for your other PR #910

After these changes we can merge this PR

@akeeste
Copy link
Contributor

akeeste commented Aug 17, 2022 •
edited
Loading

TODO:

  • Compare complex amplitude of the spectra
  • Compare how the timeseries elevation comes out in WEC-Sim when using each spectra

Thanks @dforbush2 for suggesting these comparisons as well. Unfortunately I was incorrect and MHKiT's elevation_spectrum function does not return phase information. And it seems the SciPy function that it eventually calls (scipy.signal.welch) actually cannot return phase information. It would be nice to eventually push users to MHKiT for this, but it won't meet this need for now. I will push your function back into the branch and go from there.
timeseries_compare

@kmruehl
Copy link
Collaborator

kmruehl commented Aug 24, 2022

@dforbush2 @akeeste is this ready for a merge?

@akeeste akeeste merged commit fcb90e3 into WEC-Sim:dev Aug 29, 2022
@dforbush2 dforbush2 mentioned this pull request May 17, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akeeste akeeste akeeste requested changes

Assignees

@akeeste akeeste

Labels

Wave Class Wave Classs (waveClass.m)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dforbush2 @akeeste @kmruehl