Skip to content

Conversation

@MShabara
Copy link
Contributor

@MShabara MShabara commented Oct 5, 2024

Allows users to set user defined cg and cb and prints the non hydro bodies info to the command window.

@jtgrasb
Copy link
Contributor

jtgrasb commented Oct 9, 2024

@MShabara I confirmed that your changes work as intended and let the user specify the cg and cb, but I don't think they should be added as is. The center of gravity in BEM runs is used as the center of rotation and impacts the hydrodynamic coefficients in rotational dofs. The center of buoyancy changes based on displaced volume, which also impacts hydrodynamic coefficients. In both of these cases, BEM should be rerun. If the user has a good reason to modify them without rerunning BEM, it can still be done in BEMIO, but I don't think it should be made any easier.

Happy to discuss in more detail and curious what the rest of the team thinks at the next meeting.

@jtgrasb
Copy link
Contributor

jtgrasb commented Oct 21, 2024

@MShabara We talked about this on the Sandia side and here are the two main points to summarize our discussion:

  1. Allowing users to modify the cg and cb in WEC-Sim is dangerous. Changing either of these variables should require rerunning BEM except for fringe cases. Although a warning message is helpful, it is easy to overlook and will likely lead to users mistakenly changing the cg or cb in cases where BEM should be rerun. We would like to avoid making this easier.
  2. There are fringe cases where changing the cg and/or cb may be applicable without needing to rerun BEM. For these cases, we suggest a discussion thread which details why changing cg or cb is an error by default, the cases where it may be applicable without rerunning BEM, and best practice for getting around the error (e.g., modifying the h5 or editing the source code).

@jtgrasb
Copy link
Contributor

jtgrasb commented Nov 1, 2024

@MShabara See discussion #1354. Feel free to modify this PR to only include the nonhydro body print statement, and I can review it.

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.

2 participants