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

Improvements to the structure of stokes_waves_K.H #1403

Merged
merged 24 commits into from
Dec 19, 2024

Conversation

mpolimeno
Copy link
Contributor

@mpolimeno mpolimeno commented Dec 11, 2024

Summary

Improving the structure of stokes_waves_K.H.
This PR builds on the improvements introduced in 30fc9a1 for the computation of the free surface profile, and updates the formulation for the velocity components within the context of Stokes Waves. The major change introduced here is the refactoring of the velocity computation following Kinnas's notes https://www.caee.utexas.edu/prof/kinnas/ce358/oenotes/kinnas_stokes11.pdf based on Fenton's https://johndfenton.com/Papers/Fenton90b-Nonlinear-wave-theories.pdf

Pull request type

Please check the type of change introduced:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

Checklist

The following is included:

  • new unit-test(s)
  • new regression test(s)
  • documentation for new capability

This PR was tested by running:

  • the unit tests
    • on GPU
    • on CPU
  • the regression tests
    • on GPU
    • on CPU

Additional background

Issue Number:

@mpolimeno
Copy link
Contributor Author

The basic more "cosmetic" changes are essentially done. Need to refactor the functions that compute the horizontal and vertical velocity components a bit, but I will write some small unit tests for eta and the velocities themselves first.

@mpolimeno
Copy link
Contributor Author

mpolimeno commented Dec 17, 2024

Wrote a small unit test for the free surface profile. I will revisit it and update it, if needed, as I keep going through Fenton (https://johndfenton.com/Papers/Fenton90b-Nonlinear-wave-theories.pdf) and Kinnas (https://www.caee.utexas.edu/prof/kinnas/ce358/oenotes/kinnas_stokes11.pdf). Need to add a test for the velocity components before refactoring.

@mpolimeno
Copy link
Contributor Author

The unit test for the velocity is meant also as a template for the refactoring of its computation in stokes_waves_K.H

@mbkuhn
Copy link
Contributor

mbkuhn commented Dec 18, 2024

Is this ready for review, or are there some things you are still working on adding?

@mpolimeno
Copy link
Contributor Author

mpolimeno commented Dec 18, 2024

Is this ready for review, or are there some things you are still working on adding?

@mbkuhn Almost there, I think. The last round of changes to test_waves.cpp incorporates the kind of structure that I had in mind to refactor the computation of the velocity in stokes_waves_K.H. If that structure looks sensible to you, I'll go ahead and incorporate it in the header file and then mark the PR as ready for an official review.

@mpolimeno
Copy link
Contributor Author

Reverted a few of the changes introduced in [5f27f99] for now as a temporary fix. In any case, the updated formulation is consistent and has been tested locally for all the relevant Stokes Orders. Will prioritize the refactoring of the velocity computation in the header file for the remainder of this PR.

@mpolimeno
Copy link
Contributor Author

mpolimeno commented Dec 19, 2024

This should be it for now. Further improvements/changes to the structure of the code will be the subject of a separate PR if needed

@mpolimeno mpolimeno marked this pull request as ready for review December 19, 2024 18:12
@mpolimeno mpolimeno assigned mbkuhn and unassigned mbkuhn Dec 19, 2024
@mpolimeno mpolimeno requested a review from mbkuhn December 19, 2024 18:12
@mbkuhn mbkuhn enabled auto-merge (squash) December 19, 2024 20:36
@mbkuhn mbkuhn merged commit 0e1bafb into Exawind:main Dec 19, 2024
15 checks passed
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.

3 participants