Rename and improve freq parameter in the edc module#152
Conversation
| if n_channels > 1: | ||
| for ch in range(n_channels): | ||
| reshaped_array = np.reshape( | ||
| data[ch, :n_samples_actual[ch]], | ||
| (n_blocks_min, n_samples_per_block[ch])) | ||
| time_window_data[ch, :] = np.mean(reshaped_array, axis=-1) | ||
| time_vector_window = ( | ||
| (0.5+np.arange(0, n_blocks_min)).reshape(1, -1) * ( | ||
| n_samples_per_block/sampling_rate).reshape(-1, 1)) | ||
| else: | ||
| reshaped_array = np.reshape( |
There was a problem hiding this comment.
I'm not entirely satisfied with this cumbersome if statement, so I would appreciate a suggestion for improvement
| if len(np.atleast_1d(smoothing_parameter)) > 1: | ||
| output = _intersection_time_lundby( | ||
| time_window_data[ch], noise_estimation[ch], energy_data[ch], | ||
| np.squeeze(np.atleast_2d(time_vector_window)[ch, :]), | ||
| dB_above_noise, n_intervals_per_10dB, | ||
| use_dyn_range_for_regression, sampling_rate, | ||
| ch, failure_policy) | ||
| else: | ||
| output = _intersection_time_lundby( |
There was a problem hiding this comment.
I'm not satisfied with this if statement either, so feel free to suggest something else :)
h-chmeruk
changed the title
freq paramter in the edc modulefreq parameter in the edc module
h-chmeruk
requested review from
a team,
ahms5,
artur-pa,
f-brinkmann,
hoyer-a,
mberz and
sbch22
…cept an array-like `smoothing_parameter`.
…ggestion in the PR #154 by @f-brinkmann
h-chmeruk
force-pushed
the
fix/rename_freq_parameter
branch
from
81bd8a1 to
c4769ff
Compare
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
When testing, I found a use case that breaks the code. A Signal with cshape (2, 2) that contains two bands (first dimension) and two channels (second dimension)
import pyfar as pf
import pyrato
cdim = 2
rir = pf.signals.files.room_impulse_response(crop_noise_tail=False)
if cdim >= 1:
rirs = rir.copy()
rirs = pf.utils.concatenate_channels((rirs, rirs, rirs, rirs), 0)
if cdim == 2:
rirs = rirs.reshape((2, 2))
print(rirs)
pyrato.edc.intersection_time_lundeby(rirs, smoothing_paramter=[1e3, 4e3])I am not sure if the current solution where _smooth_rir is used outside the for-loop can work, because the shape of time_window_data and time_vector_window depends on the smoothing_parameter. Maybe it should be moved inside the for-loop. But I did not check this in detail.
| smoothing_parameter : int or array_like of int or {'broadband'} | ||
| Used to determine the smoothing time window in the Lundeby | ||
| algorithm. It should represent the center frequency (in Hz) of the | ||
| frequency band(s) in which the RIR data was computed. |
There was a problem hiding this comment.
It would be good to mention that length must match data.cshape[0] if it is an array like
| The frequency band. If set to 'broadband', | ||
| the time window of the Lundeby-algorithm will not be set in dependence | ||
| of frequency. | ||
| smoothing_parameter : int or array_like of int or {'broadband'} |
There was a problem hiding this comment.
Does this require integer values? We can maybe also write
| smoothing_parameter : int or array_like of int or {'broadband'} | |
| smoothing_parameter : scalar, array_like, or {'broadband'} |
| else: | ||
| freq_dependent_window_time = (800/freq+10) / 1000 | ||
| raise TypeError( | ||
| "smoothing_parameter must be an int or array_like of int " |
There was a problem hiding this comment.
See above, would suggest
| "smoothing_parameter must be an int or array_like of int " | |
| "smoothing_parameter must be a scaler, array_like, " |
|
|
||
| if freq == "broadband": | ||
| # number of frequency bands given by first channel axis | ||
| n_bands = np.prod(data.cshape) |
There was a problem hiding this comment.
I think it should be
| n_bands = np.prod(data.cshape) | |
| n_bands = data.cshape[0] |
because our filter routines will prepend a dimension containing the bands. This means we force the user to always provide the bands in the first dimension.
| freq_dependent_window_time = (800/freq+10) / 1000 | ||
| raise TypeError( | ||
| "smoothing_parameter must be an int or array_like of int " | ||
| "or {'broadband'}") |
There was a problem hiding this comment.
| "or {'broadband'}") | |
| "or 'broadband'") |
| def energy_decay_curve_truncation( | ||
| data, | ||
| freq='broadband', | ||
| smoothing_parameter='broadband', |
There was a problem hiding this comment.
We have to deprecate the freq parameter for backwards compatability. This can be done with the pyfar._utils.rename_arg decorator. You can search pyfar for example use cases of this :)
3 participants
Which issue(s) are closed by this pull request?
Closes #123
Changes proposed in this pull request:
freqparameter tosmoothing_parametereverywhere in the code, including in the filesexamples/energy_decay_curves_and_reverberation_time.ipynbandtests/test_data/test_notebook.ipynbsmoothing_parameterdsp._smooth_rir(data, sampling_rate, smooth_block_length=0.075)so thatsmooth_block_lengthcan be an array-like value.test_intersection_time_smoothing_parameter_error()andtest_intersection_time_smoothing_parameter_array_like()