Skip to content

Read_AQWA Update #694

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
kmruehl merged 17 commits into from
Oct 13, 2021
Merged

Read_AQWA Update #694

kmruehl merged 17 commits into from
Oct 13, 2021

Conversation

@jtgrasb
Copy link
Contributor

@jtgrasb jtgrasb commented Sep 1, 2021

I added AQWA example cases to match what we have for WAMIT, Capytaine, and NEMOH. I also updated the Read_AQWA function to work for all example cases and multiple versions.

@jtgrasb jtgrasb changed the base branch from master to dev September 1, 2021 17:51
@akeeste akeeste added BEM/BEMIO related to BEMIO or BEM hydro data Feature new feature request labels Sep 2, 2021
@kmruehl kmruehl requested a review from nathanmtom September 8, 2021 14:20
nathanmtom
nathanmtom reviewed Sep 10, 2021
View reviewed changes
Copy link

@nathanmtom nathanmtom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@jtgrasb We have been wanting to add additional AQWA example files and update the BEMIO AQWA function to handle the latest changes to the AQWA output. I have never used AQWA so being able to read and provide input on the .AH1, .LIS, and .dat files are limited and my only request are if there are any input files you can add to these examples cases to illustrate what features were turned on when AQWA is run? If there are not input files can we list the common options utilized to generate these files, perhaps in a readMe.txt format? I only ask is that @dforbush2 and I have updated the BEMIO_AQWA file before and struggle sometimes to know if the errors are from changes in the AQWA output format or if the user used an input file with several different/new options.

Second, when you mention that Read_AQWA can import multiple version, how did you check this? It would be helpful if we also add a TEST case in bemioTest.m where we can check older AQWA outputs.

Third, have you had a chance to compare the hydrodynamics between WAMIT and AQWA for the same example case? There was some discussions earlier this year that we might have an error in the hydrodynamics pulled from AQWA, but the people who identified the problem never seemed to follow-up or respond to our inquiries. If we compare the .h5 files for the same case we should pretty quickly be able to see if there is an error in the signs of the hydrodynamic coefficients.

Lastly, it does look like there is a conflict in the bemioTest.m that needs to be resolved. After this, I see no problem incorporating these changes into the Dev branch as we work to address my other comments above which are more related to developing additional checks and better cataloguing these changes to make it easier in the future.

Let me know if you have any questions, comments, or other.

@jtgrasb
Copy link
Contributor Author

jtgrasb commented Sep 13, 2021

@nathanmtom Thank you for the comments!

  1. I will look through the AQWA files and figure out which holds the input files and try to include the inputs in a clear way.
  2. I initially included some tests of older versions in the bemioTest function, but then discussed with Adam and Kelley and decided that we only say we support the current version and it would be hard to provide support for a version that we don't have access to directly. I think this is something to discuss more with the team so I will bring it up at the team meeting on Wednesday.
  3. I hadn't spent much time comparing the outputs to WAMIT cases, but I will look into that.
  4. I think the conflict is only because I changed the bemioTest.m script rather than an actual error, but I will try to resolve that.

@nathanmtom
Copy link

nathanmtom commented Sep 13, 2021

@jtgrasb Thanks for your responses as they are helpful to understand the decisions made when drafting this pull request.

Re: Comments

  1. This is definitely something that would be helpful as we have the input files for WAMIT, NEMOH, and Capytaine and so if we have the ability to either place the input files or at least describe the options selected within the AQWA interface that would be helpful; however, again I've never used AQWA so perhaps it is the standard template or options.
  2. That is a fair point and have no problem committing to that moving forward. I would only request adding a not under user manual -> advanced features -> BEMIO where we introduce Read_AQWA that we put a note that states we are only supporting version X onward or we only support the latest version of AQWA which is X.
  3. I think this would be helpful to make sure the hydro coefficients are reasonably the same across all BEM software for the same geometry. They will likely be different a bit, but I'm assuming our team would like to make sure if a user created a WEC-Sim run for say RM3 using WAMIT, NEMOH, AQWA, or Capytaine that the response would all be similar and the only difference between runs in the hydrodynamics.
  4. Got it, it looks like you pulled dev into your branch and resolved the issue.

Happy to discuss any other questions or comments you have during the next WEC-Sim development team meeting.

@jtgrasb
Copy link
Contributor Author

jtgrasb commented Sep 13, 2021

@nathanmtom
I will work on a guide for the inputs soon. Unfortunately, I just realized that I mistook the frequency as being Hz rather than rad/s, and my input frequencies need to be adjusted and all of the cases reran. It may take some time to rerun these cases so I will come back to this PR once that is done.

@jtgrasb jtgrasb marked this pull request as draft September 22, 2021 14:40
@jtgrasb
Copy link
Contributor Author

jtgrasb commented Oct 11, 2021

The example cases are taking extra time to ensure they are running properly, so I think it is best to implement this PR with the Read_AQWA updates now (seeing issue #734). This PR now contains one AQWA example case for the RM3 device, an update to the bemioTest.m script, and an update to the Read_AQWA.m function which solves issue #734. I will keep working on the example cases and make another PR in the future.

@jtgrasb jtgrasb marked this pull request as ready for review October 11, 2021 14:44
@jtgrasb jtgrasb mentioned this pull request Oct 11, 2021
Closed
@jtgrasb jtgrasb linked an issue Oct 11, 2021 that may be closed by this pull request
[WEC-Sim Applications] Read_AQWA error line 134 #734
Closed
@jtgrasb jtgrasb changed the title AQWA Examples Read_AQWA Update Oct 13, 2021
@kmruehl kmruehl added the Bug bug in WEC-Sim source, high priority label Oct 13, 2021
@kmruehl kmruehl self-requested a review October 13, 2021 14:58
kmruehl
kmruehl approved these changes Oct 13, 2021
View reviewed changes
Copy link
Collaborator

@kmruehl kmruehl left a comment

Choose a reason for hiding this comment

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

@jtgrasb Thank you!

@kmruehl kmruehl merged commit fbf071d into WEC-Sim:dev Oct 13, 2021
@jtgrasb jtgrasb deleted the aqwa_test branch November 17, 2021 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kmruehl kmruehl kmruehl approved these changes

+1 more reviewer

@nathanmtom nathanmtom nathanmtom left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

BEM/BEMIO related to BEMIO or BEM hydro data Bug bug in WEC-Sim source, high priority Feature new feature request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[WEC-Sim Applications] Read_AQWA error line 134

4 participants

@jtgrasb @nathanmtom @kmruehl @akeeste