sfstoolbox / sfs-matlab

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

Fix structure of conf for LWFS methods #162

Closed fietew closed 6 years ago

fietew commented 7 years ago

Includes:

Maybe we should move conf.localsfs.wfs to conf.localsfs.vss.wfs to make this even more clearer? Same may hold for conf.localsfs.usetapwin and conf.localsfs.tapwinlen.

hagenw commented 7 years ago

Your changes look good so far.

How to write this config section in a way that it is easy to understand seems to be challenging. I'm also not sure if I understand everything at the moment. Maybe I start with a few questions/remarks.

  1. I guess conf.localsfs.vss.driving_functions defines the driving functions that will be used to drive the virtual sources (belonging to conf.localsfs.wfs) not to generate them. Then it would be indeed better to rename conf.localsfs.wfs to conf.localsfs.vss.wfs and so on.
  2. On the other hand conf.localsfs.vss.wfs has again entries and would become very long. Maybe we could rename conf.localsfs.vss.driving_functions to conf.localsfs.driving_functions?
  3. I think in general we should add a description at the beginning of this config section explaining, which setting is for the generation of the virtual sources and which setting is for the driving of the virtual sources.
  4. At the moment conf.localsfs.method can be 'wfs' or 'nfchoa', but we have only conf.localsfs.wfs not conf.localsfs.nfchoa. Might it be possible to solve this by allowing for conf.localsfs.method = conf.wfs?
fietew commented 7 years ago

I guess conf.localsfs.vss.driving_functions defines the driving functions that will be used to drive the virtual sources (belonging to conf.localsfs.wfs) not to generate them.

Yes, that's correct.

On the other hand conf.localsfs.vss.wfs has again entries and would become very long. Maybe we could rename conf.localsfs.vss.driving_functions to conf.localsfs.driving_functions?

Maybe it would be a good idea to rename the localsfs to something else, because it does not match the actual name of the LSFS methods in the function call, e.g. driving_function_imp_localwfs-sbl or driving_function_imp_localwfs-vss . Off course, this would require to different entries for localwfs-vss and localwfs-sbl, but the two methods do not have very much in common in terms of configuration.

I think in general we should add a description at the beginning of this config section explaining, which setting is for the generation of the virtual sources and which setting is for the driving of the virtual sources.

Yes, agree.

At the moment conf.localsfs.method can be 'wfs' or 'nfchoa', but we have only conf.localsfs.wfs not conf.localsfs.nfchoa. Might it be possible to solve this by allowing for conf.localsfs.method = conf.wfs?

Yes, this one is still missing and is supposed be covered by this pull request.

A suggestion covering most of the issues could be:

% === LSFS using Virtual Secondary Sources (LSFS-VSS) ===
conf.localwfs-vss.size = 0.4;
conf.localwfs-vss.center = [0, 0, 0];
conf.localwfs-vss.geometry = 'circular';
conf.localwfs-vss.number = 56;
conf.localwfs-vss.grid = 'equally_spaced_points';
conf.localwfs-vss.method = 'wfs';
conf.localwfs-vss.wfs = conf.wfs;
conf.localwfs-vss.usetapwin = false;
conf.localwfs-vss.tapwinlen = 0.5;
conf.localwfs-vss.nfchoa = conf.nfchoa;
conf.localwfs-vss.driving_functions = 'default';

% === LSFS using Spatial Bandwidth Limitation (LSFS-SBL) ===
conf.localwfs-sbl.order = [];
conf.localwfs-sbl.fc = [];
conf.localwfs-sbl.Npw = [];
hagenw commented 7 years ago

As both methods have indeed not much shared configuration it makes sense to rename localsfs. You are not allowed to use - inside a variable name. Also considering the method discussion from above it would then result in:

% === LSFS using Virtual Secondary Sources (LSFS-VSS) ===
conf.lwfs_vss.size = 0.4; % \ m
conf.lwfs_vss.center = [0, 0, 0]; % \ m
conf.lwfs_vss.geometry = 'circular'; % string
conf.lwfs_vss.number = 56; % integer
conf.lwfs_vss.grid = 'equally_spaced_points'; % string
conf.lwfs_vss.method = conf.wfs; % struct
conf.lwfs_vss.usetapwin = false; % boolean
conf.lwfs_vss.tapwinlen = 0.5; % 0..1
conf.lwfs_vss.driving_functions = 'default'; % string

% === LSFS using Spatial Bandwidth Limitation (LSFS-SBL) ===
conf.lwfs_sbl.order = []; % integer
conf.lwfs_sbl.fc = []; % integer
conf.lwfs_sbl.Npw = []; % integer

What still looks a little bit confusing is that we have the LSFS as title, but only lwfs in the configuration. Should we change the title to LWFS as well?

fietew commented 7 years ago

Yes, that was just copy&paste from the current SFS_config.m. We can adjust that. If you are okay with the structure, I will try to incorporate it into all functions etc.

hagenw commented 7 years ago

I'm fine with the structure.

hagenw commented 6 years ago

Cool, from my site that's it. I would do a clean up of the commits by some squashing and then merge this. Or do you want to add more changes?

fietew commented 6 years ago

No