sfstoolbox / sfs-matlab

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

Downsample full spectrum and use full ifft #155

Closed chris-hld closed 7 years ago

chris-hld commented 7 years ago

By using a full (symmetric) spectrum, the symmetric flag can be avoided. real() only deals with very small round-off errors now.

hagenw commented 7 years ago

As we are now using signals that are doubled in length will this slow the calculation down? For interpolate_ir() this might be important as you could imagine calling this function quite often.

By using this approach to avoid 'symmetric' altogether we also have to change this line in modal_weighting().

chris-hld commented 7 years ago

The symmetric option in combination with the number of output bins should do exactly the same. As long as this is not optimized internally, this should make no difference. There is no indication that this is the case, have a look at the computation times in https://github.com/sfstoolbox/sfs-matlab/issues/133

chris-hld commented 7 years ago

Should be patched.

hagenw commented 7 years ago

Cool, thanks. I will make a final test next week under Matlab and Octave.

hagenw commented 7 years ago

Thanks, modal_weighting() works without the symmetric option now and returns the exact same results. I added test_modal_weighting() to check for this.

@chris-hld: the new implementation of interpolate_ir() does not lead to the same results as before. Please run test_interpolation_methods(1) for the master branch and your branch and compare Fig. 2, 3, 5, and 7. There you can easily find differences.

fietew commented 7 years ago

The problem was the phase unwrapping of the second part, i.e. indices greater idx_half, of the spectrum. It is now done for the first part, only, followed by a mirroring of magnitude and phase.

hagenw commented 7 years ago

Cool, now there is now difference any longer. I would like to squash the commits into two (one for the added test_modal_weighting() and one for the changes to interpolate_ir()), before merging. Is this fine with you?

fietew commented 7 years ago

Yep

chris-hld commented 7 years ago

Sure!