fix_readWAMIT_and_writeBEMIOh5 #1065
Conversation
|
@salhus Thanks for double checking the implementation here. As I'm reviewing this PR one quick request is that in 'readBEMIOH5.m' to delete line 56 where you originally zeroed the 6 row of the hydrostatic matrix. I'll follow-up with comments directly on the files you've changed so far. |
this commit removes the line 56 that zeros the last row of the hydrostatics matrix.
|
@nathanmtom Thanks for pointing that. I just made a commit that removes the line 56. |
source/functions/BEMIO/readWAMIT.m
Outdated
| tmp = textscan(raw{n+3}(find(raw{n+3}==':')+1:end),'%f'); | ||
| hydro(F).Khs(5,5:6,hydro(F).Nb) = tmp{1}; | ||
| hydro(F).Khs(5:6,5,hydro(F).Nb) = tmp{1}; | ||
| hydro(F).Khs(6,:) = 0*hydro(F).Khs(6,:); % 6th row must be zeros |
There was a problem hiding this comment.
Hey Sal, so putting line 96 in does work, but wondering if rather than writing cells then overwriting with 0 it would be better to update how we import and assign the hydrostatic coefficients. If we change the following lines we should get the same result but adjust the code to better infer what coefficients should be transposed across the diagonal.
Replace line 92 with:
hydro(F).Khs(4:5,4,hydro(F).Nb) = tmp{1}(1:2);
Delete lines 95 and lines 96
There was a problem hiding this comment.
That makes sense. That would remove the 6th row being written in the first place. I made a commit to that effect
| clear tmp; | ||
| else | ||
| writeH5Parameter(filename,['/body' num2str(i) '/hydro_coeffs/linear_restoring_stiffness'],hydro.Khs(:,:,i),'Hydrostatic stiffness matrix',''); | ||
| writeH5Parameter(filename,['/body' num2str(i) '/hydro_coeffs/linear_restoring_stiffness'],hydro.Khs(:,:,i)','Hydrostatic stiffness matrix',''); |
There was a problem hiding this comment.
@salhus I do not have an .h5 viewer, but you mention that this flip is to make the viewer have the right matrix format; however, we then use the reverseDimensionOrder in the readBEMIOH5 to get it back in the original form for WEC-Sim. Therefore, we are flipping it forward and flipping it backward. I tried running without the transpose and then removing the reverseDimensionOrder and the WAMIT and AQWA matrices were in the right format for WEC-Sim.
Therefore, my question is if we can clean up this implementation to not have unnecessary flipping if it is only for the .h5 viewer. As mentioned above the results come out as we want, just seeing if the implementation can be modified.
There was a problem hiding this comment.
@nathanmtom,
I see your point that it is redundant. However, I think having the h5 file represent the coefficients is also important.
The h5 files are useful in their own right because it can be a standardised way of expressing the bemio processed results regardless of BEM software being used. This is helpful when comparing hydrodynamic coefficients, or importing it to optimizers etc.
There was a problem hiding this comment.
@salhus I understand and agree with that point. But now wondering why the .h5 viewer does a flip when importing into its own data format. Did you have the opportunity to verify if other .h5 viewers provided the same result? As perhaps if there is an import issue with the one .h5 viewer you are testing while others do not have this issue then perhaps we eliminate this redundancy and note one .h5 viewer is perhaps importing incorrectly. This might be a non-issue just do not personal use .h5 viewers at this time.
There was a problem hiding this comment.
@nathanmtom
Yes, I did check with another h5viewer, and observed the same dimension order.
There was a problem hiding this comment.
Understood, then no further discussion on this point needed and will keep it as is.
only_write_what_you_need
-forgot adding (1,2) in the last commit
source/functions/BEMIO/readWAMIT.m
Outdated
| tmp = textscan(raw{n+3}(find(raw{n+3}==':')+1:end),'%f'); | ||
| hydro(F).Khs(5,5:6,hydro(F).Nb) = tmp{1}; | ||
| hydro(F).Khs(5:6,5,hydro(F).Nb) = tmp{1}; | ||
| % 6th row must be zeros |
There was a problem hiding this comment.
@salhus Minor comment, can we delete this line. Without more context a user might not understand why and we do not provide greater description in our documentation. Therefore, requesting to either delete, or reference the AQWA or WAMIT manuals.
There was a problem hiding this comment.
Sounds good. I just made that commit.
remove comment for cleaner code
|
Thanks for bearing with me and answering my questions on a fairly straightforward PR, but still nice to have documentation of the decisions made if we need to revisit in the future. Merge coming shortly. |
|
Thanks @nathanmtom ! I appreciate it. |
@nathanmtom
As mentioned in the last comment for #1063 the fix needs to be done to readWAMIT and writeBEMIOh5.$(4,6)$ and $(5,6)$ terms.
Here is an illustration.
I modified an .out file to have
Here is how readWAMIT would read it to MATLAB workspace,
I then fixed the readWAMIT to have zeros in the$6^{th}$ row. The readWAMIT function then read the .out file correctly to the MATLAB workspace but the writeBEMIOh5 still automatically does a transpose (perhaps the writing indices are switched in the writing function),
Here is how readWAMIT read the .out file,
While after the writeBEMIOh5 is done the h5 file is written like this,
After having the trasnpose done in the writeBEMIOh5 as well, it can be seen that readWAMIT reads the matrix correctly to the workspace, and the writeBEMIOh5 writes the h5 file correctly,
here is readWAMIT reading the matrix correctly to the workspace,
and here is the h5 file correctly written,
