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

BEMIO Cleanup #790

Merged
merged 31 commits into from
Feb 17, 2022
Merged

BEMIO Cleanup #790

merged 31 commits into from
Feb 17, 2022

Conversation

akeeste
Copy link
Contributor

@akeeste akeeste commented Jan 26, 2022

This scripts completes the BEMIO cleanup project board task:

  • rename BEMIO variables and internal functions to camel case
  • rename BEMIO scripts to camel case
  • removed redundant writeh5 functions

updates after review:

  • changed writeH5 to writeBEMIOH5 to prevent confusion with matlab functions
  • updated H5CreateWriteAtt to writeH5Parameter since it creates a variable, writes values, and defines attributes
  • removed bemio.m and replaced it with a short README.md pointing to the examples/BEMIO cases
  • removed specific BEM names in normalizeBEM.m, it is general now
  • removed the loadH5 function since it replicates a lot of what the built in h5read does. The only purpose it served for us was to reverse the order of the dimensions upon reading from the h5 file. I'm not sure why this is necessary, but I created a short utility function that will does this same thing after using h5read

updates building on Kelley's compareBEMIO PR into this fork:

  • test plotting for different number of WEC bodies, single/double/triple, e.g. sphere, rm3, wec3
  • test plotting for single, double and triple BEMIO results, e.g. WAMIT, Capytaine, AQWA
  • test plotting when nested in plotBEMIO, resolve issues with nested varargin functions
  • remove *.mat and AQWA files
  • add examples/**/*.png to gitignore so that images of BEMIO plots are not included on the repo
  • Point bodyClass to the readBEMIOH5 function
  • refactor plotting scripts to remove the hydro parameter entirely. Also functions use varargin for simplicity
  • create new test for the compareBEMIO functionality by running plotBEMIO for two different Sphere BEM runs

Future Task --> project board

  • Compare BEM data by loading h5 data into hydro data structure. Right now, this comparison is based on a temporary hydro structure from the BEM output files

@kmruehl kmruehl self-requested a review January 28, 2022 21:21
Copy link
Contributor

@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.

Thanks Adam, LMK if you have any questions

source/functions/utils/v5Renaming.m Outdated Show resolved Hide resolved
source/functions/utils/updateNaming.m Outdated Show resolved Hide resolved
source/functions/BEMIO/writeH5/h5bemWriteBody.m Outdated Show resolved Hide resolved
source/functions/BEMIO/writeH5/h5bemWriteParameters.m Outdated Show resolved Hide resolved
source/functions/BEMIO/writeH5/h5bemCreate.m Outdated Show resolved Hide resolved
source/functions/BEMIO/waveNumber.m Outdated Show resolved Hide resolved
@kmruehl kmruehl added the BEM/BEMIO related to BEMIO or BEM hydro data label Jan 29, 2022
@akeeste
Copy link
Contributor Author

akeeste commented Jan 31, 2022

@kmruehl I made the requested changes, namely:

  • removed renaming scripts. I can't find away to attach files to a team discussion thread, but have them locally if needed
  • moved the waveClass.waveNumber method to the BEMIO/waveNumber.m function so that it's accessible by both Read_NEMOH and waveClass
  • removed redundant write H5 functions. These have been incorporated into writeH5.m previously

@kmruehl
Copy link
Contributor

kmruehl commented Feb 9, 2022

  • What's the difference between h5read and loadH5? Both used in bodyClass
  • Naming convention for h5write and writeH5, distinguish between built-in MATLAB and BEMIO functions
  • internal functions, e.g. H5CreateWriteAtt
  • What is purpose of bemio.m? Should we keep it and add description? Remove it and point to examples?
  • normalizeBEM update description to include Capytaine or remove BEM names

@akeeste
Copy link
Contributor Author

akeeste commented Feb 10, 2022

@kmruehl Thanks for discussing yesterday. I made the latest suggested changes, namely:

  • changed writeH5 to writeBEMIOH5 to prevent confusion with matlab functions
  • updated H5CreateWriteAtt to writeH5Parameter since it creates a variable, writes values, and defines attributes
  • removed bemio.m and replaced it with a short README.md pointing to the examples/BEMIO cases
  • removed specific BEM names in normalizeBEM.m, it is general now
  • I removed the loadH5 function since it replicates a lot of what the built in h5read does. The only purpose it served for us was to reverse the order of the dimensions upon reading from the h5 file. I'm not sure why this is necessary, but I created a short utility function that will does this same thing after using h5read

@kmruehl
Copy link
Contributor

kmruehl commented Feb 15, 2022

@akeeste PR #11 should resolved bug (unchecked boxes above) with the nested varargin. Should we add/update bemio tests for this?

kmruehl and others added 7 commits February 15, 2022 09:33
* Fix AQWA data for sphere, ellipsoid, and WEC3

* working on compare BEM hydro data

* combined added mass BEM

* compare added mass complete

* removing duplicate files

* updating plot radiation damping

* updating plot radiation irf

* updating plot exc mag

* updated plot excitation phase and irf

* updated plot bemio scripts

* reverting bemio examples

* updating plot functions to camelCase

* resolving conflicts

* updating compare plots for multiple body wecs

* extending plotAddedMass to multiple hydro datasets

* extending plotRadDamping to multiple hydro datasets

* adding extensible plot rad irf

* adding exc mag and phase

* updaing exc irf plot for mult bemio results

* resolve radiationIRF SS bug

* cleaning up compareBEM

* resolving nested varargin for plotAddedMass

* resolving nested varargin for all plots

* nested varargin bug resolved

* updating compareBEM to camelCase

* resolving plotExcIRF bug

* removing pngs

Co-authored-by: jtgrasb <jtgrasb@sandia.gov>
Co-authored-by: Adam Keester <72414466+akeeste@users.noreply.github.com>
@akeeste
Copy link
Contributor Author

akeeste commented Feb 16, 2022

@kmruehl I have finished the last clean up items on this PR. Everything seems to be working now and all tests pass locally. I went through all the files changed and don't see anything unintentional. I'm happy with merging this PR and then working on new tests/documentation separately.

@kmruehl
Copy link
Contributor

kmruehl commented Feb 16, 2022

@akeeste I'll review this again and merge it. I can work on updating the tests, if you want to work on the documentation updates. We can work off the same PR though, I'll open one and tag you in it once this is merged.

Copy link
Contributor

@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.

Note to self, double check the waveNumber calculation for shallow, deep and intermediate water

@kmruehl kmruehl merged commit 0e04777 into WEC-Sim:dev Feb 17, 2022
@kmruehl
Copy link
Contributor

kmruehl commented Feb 17, 2022

@akeeste thanks! I'll leave it to you to delete your own branch if you like.

@akeeste
Copy link
Contributor Author

akeeste commented Feb 21, 2022

@akeeste I'll review this again and merge it. I can work on updating the tests, if you want to work on the documentation updates. We can work off the same PR though, I'll open one and tag you in it once this is merged.

This sounds good. Wednesday before seeing this I started on reorganizing these tests to be more efficient. Let me know when you start and we could use that branch as a jumping off point https://github.com/akeeste/WEC-Sim/tree/bemioTestDocs

@akeeste akeeste deleted the bemioCleanup branch February 21, 2022 16:53
@kmruehl
Copy link
Contributor

kmruehl commented Feb 22, 2022

@akeeste I already updated the tests in this PR. However, I did not restructure them, I just added a test for the new plot feature. If you'd like to submit your restructured tests as a PR, feel free to.

@akeeste
Copy link
Contributor Author

akeeste commented Feb 22, 2022

@kmruehl Okay got it. I'll submit some restructuring when I update BEMIO documentation. I'm hoping to hold on to the wamit/nemoh/other hydrodata whenever it is read in to save time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BEM/BEMIO related to BEMIO or BEM hydro data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants