Multi-heading Seas #1123
Multi-heading Seas #1123
Conversation
There was a problem hiding this comment.
@salhus can you double check what advanced_features.rst you are amending. Below it looks like you are missing a lot of the recent control documentation updates.
There was a problem hiding this comment.
@nathanmtom
Hi Nathan,
I will update this file once the other PR is merged. I will address your other comments meanwhile.
| .. Note:: | ||
| If using a wave-spectra with different wave-heading directions, ensure that the BEM data has | ||
| the hydrodynamic coefficients corresponding to the desired wave-heading direction. | ||
|
|
There was a problem hiding this comment.
@salhus Editorial Comment: can you please expand the existing note or add another note highlighting what combinations of features are compatible at this time. Based on theinitializeWECSim.m edits this would only be functional for regular, regularCIC, irregular, and spectrumImport.
Also clarify if the waveClass needs to be the same i.e. regularCIC and regularCIC or can there be a mix i.e. regularCIC and irregular.
source/functions/initializeWecSim.m
Outdated
| body(it).hydroForcePre(waves.omega,waves.direction,simu.cicTime,waves.bem.count,simu.dt,... | ||
| simu.rho,simu.gravity,waves.type,waves.waveAmpTime,simu.stateSpace,simu.b2b); | ||
| body(it).hydroForcePre(waves(1).omega,waves(1).direction,simu.cicTime,waves(1).bem.count,simu.dt,... | ||
| simu.rho,simu.gravity,waves(1).type,waves(1).waveAmpTime,simu.stateSpace,simu.b2b); % TODO need to rethink how user defined exc is implemented for multiple waves |
There was a problem hiding this comment.
@salhus Minor comment: can you remove the comment and add as a project board card. That should be relatively straightforward to allow user defined waves but will not prioritize it until next fiscal year.
source/functions/initializeWecSim.m
Outdated
| it = idx(kk); | ||
| body(it).hydroForcePre(waves.omega,waves.direction,simu.cicTime,waves.bem.count,simu.dt,... | ||
| simu.rho,simu.gravity,waves.type,waves.waveAmpTime,simu.stateSpace,simu.b2b); | ||
| body(it).hydroForcePre(waves(1).omega,waves(1).direction,simu.cicTime,waves(1).bem.count,simu.dt,... |
There was a problem hiding this comment.
@salhus Moderate comment: Is there a reason that waves(1) is hard coded to the first wave instance? This seems to indicate that you cannot mix and match certain wave features. This relates to my request on documentation to state what combinations of sea states are possible.
There was a problem hiding this comment.
@nathanmtom
Yes that is correct. I think this can be expanded and I am planning on enabling mix-n-match in next FY.
I will update documentation to reflect this limitation.
There was a problem hiding this comment.
Minor Comment: can the comments be removed below to not add lines 525-527? This seems to have no impact on this file other than commented lines. I would prefer not to have this included in the list of changed files.
There was a problem hiding this comment.
This was a remnant from previous version. I uncommented it and a commit-name will reflect this.
|
@salhus Thanks for the PR which is going to be a good addition. I am not done reviewing, but wanted to make this point before I forget. Your two PRs both have library edits which means we are likely going to have to merge one and you will then have to take the merged library and re-incorporate you modifications so all changes to the library are captured. Just wanted to make you aware as we plan to merge both FIR and N-Wave PRs this week. |
There was a problem hiding this comment.
@salhus When I open this library it appears the default mask shows two separate wave trains already. Please review and see if this can be resolved with re-saving the library with only one instance.
There was a problem hiding this comment.
@nathanmtom
Good catch Nathan. I have fixed this issue now.
There was a problem hiding this comment.
@salhus When I open the body elements there seems to be two wave trains already present which may be a consequence of saving the library with both active. Please double check if this is intended or the library can be saved with one wave train present.
|
@nathanmtom |
This follows from #1070 for which GitHub is not performing tests and I was having compatibility issues.