Skip to content

Conversation

@salhus
Copy link
Contributor

@salhus salhus commented Apr 14, 2023

image

The roll mode rows should be 0.
The readBEMIOh5 function uses the permute command to make matrix dimensions compatible to WEC-Sim. This essentially flips the matrix, such that, $X(1:nFreq, b, a)$ becomes $X(a, b, 1:nFreq)$. This works well due to matrix symmetry. However, the roll row for linear restoration matrix must be 0.
This follows from Issue #1028

@salhus salhus requested a review from nathanmtom April 14, 2023 20:15
% Read hydrostatic stiffness
hydroData.hydro_coeffs.linear_restoring_stiffness = reverseDimensionOrder(h5read(filename, [h5BodyName '/hydro_coeffs/linear_restoring_stiffness']));

hydroData.hydro_coeffs.linear_restoring_stiffness(6:6:end,:) = 0*hydroData.hydro_coeffs.linear_restoring_stiffness(6:6:end,:);

Choose a reason for hiding this comment

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

@salhus Is there a reason you are using the (6:6:end,:) rather than (6,:) as you are just trying to zero the last row of the 6x6 hydrostatic stiffness matrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @nathanmtom
I left that as a safe-guard thinking if the matrix was $6N\times6N$.
I just made a commit that only does that to the $(6,:)$

Choose a reason for hiding this comment

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

@salhus Thanks for the clarification, I see why you chose to implement the prior option; however, I do believe that when this function is called it requires the body number and should only have a 6x6 hydrostatic matrix upon import. If you can double check this, I think sticking with the (6,:) is easier to read, but if we are indeed getting multiple bodies read in then we should stick with your implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nathanmtom
Thanks for pointing this out. The imports do happen on a body by body basis, so the $(6,0)$ elements being zeroed makes more sense.

@nathanmtom
Copy link

@salhus Thanks for looking into the issue and submitting a fix. I've verified that the additional line of code you have included will zero out the last row of the 6x6 hydrostatic restoring matrix. Which is consistent that the yaw-roll and yaw-pitch coupling terms in the 6,4 and 6,5 indices are zero based on the WAMIT documentation you have cited. Please review my one comment above on how you are handling the matrix indices as my recommendation seems more direct, but I might not understand the reason behind that implementation. After we resolve that minor comment this would be fine to merge.

only change the last row, since the restoring matrix is always 6x6
@nathanmtom
Copy link

Thanks @salhus for confirming that the function imports body hydrostatics one at a time. I am happy with the implementation and will merge this PR.

@nathanmtom nathanmtom merged commit ffa3e03 into WEC-Sim:dev May 10, 2023
@salhus
Copy link
Contributor Author

salhus commented May 11, 2023

@nathanmtom Thanks Nathan.

@salhus salhus deleted the fix-for-linear-restoring-matrix branch May 11, 2023 15:17
@akeeste akeeste mentioned this pull request May 17, 2023
6 tasks
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