search

sfstoolbox / sfs-matlab

SFS Toolbox for Matlab/Octave
https://sfs-matlab.readthedocs.io
MIT License
97 stars 39 forks source link

Replace some code for NFC-HOA with new functions #141

Closed fietew closed 7 years ago

fietew commented 7 years ago

As some new functions have been added in #135, they could be added to shorten the code a little. Major changes for the user: Output dm is transposed (first dim: time, second dim: modal order)

hagenw commented 7 years ago

It seems to give the same result, so it should be fine. Regarding the transposed dm, I think it is better in the new way as we normally always have time as first dimensions, haven't we?

But I have a question regarding the function header as we have now some inconsistencies. For the secondary sources and the driving signal you are using now:

%       x0      - position  and direction of secondary sources [N0x7] / m
%       d       - matrix of driving signals [NxN0]

whereas in driving_function_imp_wfs() we use:

%       x0      - positions and directions of secondary sources / m [nx6]
%       d       - driving signals [mxn]

So beside the difference of the x6 for WFS there is inconsistency by using n, N, and N0. Maybe we create another pull request where we discuss the nomenclature for such dimension descriptions and then change the headers accordingly.

fietew commented 7 years ago

Yeah, we should start a general discussion about that.

Regarding the x6: This is actually wrong, isn't it? https://github.com/sfstoolbox/sfs-matlab/blob/0f6987721ae1f07b5275d41cae95647e06eeb7fe/SFS_time_domain/driving_function_imp_wfs.m#L65 rejects inputs with size(x0,2) ~= 7.

hagenw commented 7 years ago

You are right. I changed it. Now the text is maybe not complete as it talks only of position and direction and not about the 7th dimension, but we can change this later.