sfstoolbox / sfs-matlab

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

Remove conf.N setting #90

Closed hagenw closed 8 years ago

hagenw commented 8 years ago

This resulted from a discussion in #89.

The idea is that the user don't have to specify the desired impulse response link with conf.N, but that the Toolbox automatically decides what is the best length.

This would be beneficial, especially for new users. The only downside I see at the moment is that the resulting impulse responses could be very long if the original data from the SOFA file is long.

hagenw commented 8 years ago

@VeraE: I have updated ir_correct_distance() and removed conf.N there. Could you have a look if this is still ok, please.

hagenw commented 8 years ago

I started working on removing conf.N from the time-domain driving functions. For WFS it should work now.

@VeraE, @fietew: for wfs_preequalization() and driving_function_imp_nfchoa() I'm not sure how to do it. 1.) For driving_function_imp_nfchoa() the size is specified at line 88 and it will not work if there are no zeros padded at the end. But I don't know how to calculate how many zeros we need. 2.) For wfs_preequalization() zeros are added at line 83 for IIR filtering, is this needed and how we can replace it?

@fietew: could you have a look at driving_function_imp_localwfs() and driving_function_imp_wfs_vss() and see if you can replace conf.N there in a similar way I have done it for driving_function_imp_wfs()?

VeraE commented 8 years ago

I did some updates on ir_correct_distance(). Now there remains the problem that in ir_generic() the impulse responses that are delivered by get_ir() could have different lengths but should be saved in one array. At the moment the impulse responses are first convoluted and then truncated to conf.N, so calculations finish succesfully. Getting rid of the conf.N restriction here would mean to either pre-calculate the maximum delay or adapt the size of the array in every loop over x0. I fear both options would increase computation time. Which do you think we should try first?

hagenw commented 8 years ago

Mh, too bad, this does not sound like there is an easy solution. I'm a little bit afraid of your second approach and changing sizes inside a loop is normally a bad idea in Matlab. The first one would require something very similar to #67, which also would require a pre-scanning of the needed impulse responses. So far I was not very motivated to do this as I was afraid that the code of ir_generic() would not be easy to understand afterwards.

Do you still think it is worse trying to remove conf.N also from ir_generic()? In my opinion it would be ok, to only use it there (and maybe rename it). In this way we would also maintain the nice possibility of shortening the impulse response in ir_generic().

VeraE commented 8 years ago

You are right both approaches are not a good idea.

To me it doesn't make sense to only remove conf.N in some parts of the calculation, though. As long as there is one step where impulse responses are shortened the user has to know before how to set conf.N sensibly. I was hoping we could come up with a solution that would not make the code much more complicated or slower. Unfortunately, I couldn't think of a better way to implement this so far. So maybe, we have to drop this completely?

hagenw commented 8 years ago

For the impulse responses part we could still think about if it is a good idea to not include it into get_ir() and ir_correct_distance() or if we should stay with the old approach. For me both would be fine. At the current master version get_ir() returns already a length of conf.N (only for the HRIR case, for BRIR we have to modify it).

But I'm still very interested in getting rid of conf.N from all the time-domain driving functions. There should be no need for the user to specify it there.

VeraE commented 8 years ago

Fiete has just helped me to fix this branch as I managed to mess up the three commits you did before my merge.

So your idea would be to have truncating to conf.N at a higher level in the code? We can do that. So I'd leave ir_correct_distance() as it now in this branch and have a look at the commits done by you for the driving functions.

VeraE commented 8 years ago

Time-domain driving function for WFS looks fine. I just got one question: why is the impulse response zero-padded by delay_offset as well in line 140 of driving_function_imp_wfs()? I'd think this is unnecessary but maybe I am missing something here...?

The WFS IIR pre-equalization does not work properly without the zero-padding as the Matlab function sosfilt() in wfs_prequalization() is returning a signal with the same length as its input signal (which is 1 when called from driving_function_imp_wfs()). We would have to specify a new parameter here if conf.N is not used, I fear.

I had a look at driving_function_imp_nfchoa() too, but have no idea how to precalculate the zeros either at the moment. I've never used this function extensively.

hagenw commented 8 years ago

With the delay_offset you are probably right. I was not sure, so I have been gone for the safe choice.

As it also seems complicated to remove conf.N from the driving functions, we maybe give up on it altogether. I will wait for @fietew opinion.

fietew commented 8 years ago

@VeraE, @fietew: for wfs_preequalization() and driving_function_imp_nfchoa() I'm not sure how to do it. 1.) For driving_function_imp_nfchoa() the size is specified at line 88 and it will not work if there are no zeros padded at the end. But I don't know how to calculate how many zeros we need. 2.) For wfs_preequalization() zeros are added at line 83 for IIR filtering, is this needed and how we can replace it?

It's an IIR-Filter. You theoretically need an infinite number of zeros. Maybe we could truncate the impulse responses if the power/amplitude of the last coefficient is below a given threshold.

@fietew: could you have a look at driving_function_imp_localwfs() and driving_function_imp_wfs_vss() and see if you can replace conf.N there in a similar way I have done it for driving_function_imp_wfs()?

There is no driving_function_imp_wfs_vss(), btw. I think the current implementation does not take the delay offset caused by the fractional delay filter into account. If necessary, I could implement a function which returns just the delay of the chosen fractional delay method.

hagenw commented 8 years ago

@fietew: Here you can find driving_function_imp_wfs_vss(). At line 64 conf.N is used. And in driving_function_imp_localwfs() at line 84.

And do we have to account for the delay offset of the fractional delay filter for specifying the driving signal length? For the normal WFS case I'm considering now only the maximum delay without possible delay offsets.

fietew commented 8 years ago

I discussed with @trettberg and we somehow agreed, that keeping conf.N is a more consistent way than perhaps dealing with driving signal of different lengths. Imho this can be closed.

hagenw commented 8 years ago

OK, I agree.