sfstoolbox / sfs-matlab

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

Unify the two LWFS methods #140

Closed fietew closed 7 years ago

fietew commented 7 years ago

As already lined out in #135, some synergies between the LWFS methods could be used to shorten the code.

TODOs (* stands for driving_function_):

hagenw commented 7 years ago

Is this ready for review, or will you add more commits?

fietew commented 7 years ago

I think, it is ready for review

hagenw commented 7 years ago

The is an error in driving_function_mono_localwfs_vss() the return value idx is not assigned inside the function.

fietew commented 7 years ago

I can not reproduce this error. Can you post a code snippet?

hagenw commented 7 years ago
>> test_localwfs(0)            
Warning: DRIVING_FUNCTION_MONO_LOCALWFS: this function name is deprecated and will be removed in future releases. Use driving_function_mono_localwfs_vss, instead 
> In driving_function_mono_localwfs (line 35)
  In sound_field_mono_localwfs (line 92)
  In test_localwfs (line 96) 
Error in driving_function_mono_localwfs_vss (line 65)
nargmin = 5;

Output argument "idx" (and maybe others) not assigned during call to "driving_function_mono_localwfs_vss".

Error in driving_function_mono_localwfs (line 39)
[d,x0,xv,idx] = driving_function_mono_localwfs_vss(x0,xs,src,f,conf);

Error in sound_field_mono_localwfs (line 92)
[D, x0, xv] = driving_function_mono_localwfs(x0,xs,src,f,conf);

Error in test_localwfs (line 96)
[~] = sound_field_mono_localwfs(X,Y,Z,xs,src,f,conf);

And you can just open the function in your editor and search for idx and you will see that it is included as a return value and in the function header, but not in the code.

fietew commented 7 years ago

okay, I fixed this for driving_function_mono_localwfs_vss() and added ìdx as a return value for the time domain driving function

hagenw commented 7 years ago

We still have sound_field_imp_localwfs() and sound_field_mono_localwfs(). I guess they should be both renamed to _vss? Inside them the driving function call has also to be renamed accordingly. For the mono case sound_field_mono_localwfs_sbl() is missing, but I guess this is not implemented yet?

fietew commented 7 years ago

We still have sound_field_imp_localwfs() and sound_field_mono_localwfs(). I guess they should be both renamed to _vss? Inside them the driving function call has also to be renamed accordingly.

Should I completely remove the old functions or add a warning msg. as done for e.g. driving_function_mono_localwfs()?

For the mono case sound_field_mono_localwfs_sbl() is missing, but I guess this is not implemented yet?

Correct.

hagenw commented 7 years ago

Maybe is a good idea to do one release with a warning message and then remove the function later.

hagenw commented 7 years ago

As we have also some commits from an already merged version in this pull request, is it ok if I do a squash and merge?

fietew commented 7 years ago

yes, squash sounds fine to me