V1 parameter definition #80
Conversation
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
Thanks. Looking good already. Only small suggestions and a structural question for discussion.
| # Check input type | ||
| if not isinstance(energy_decay_curve, pf.TimeData): | ||
| raise TypeError("Input must be a pyfar.TimeData object.") | ||
| if not isinstance(early_time_limit, (int, float)): | ||
| raise TypeError('early_time_limit must be a number.') |
There was a problem hiding this comment.
If these checks and the one checking for real-valued data are in most parameter functions, we could move them to a separate private function to reduce redundancy. Could be done in a separate pull, once more parameters are implemented
| if energy_decay_curve.complex: | ||
| raise ValueError( | ||
| "Complex-valued input detected. Definition is" | ||
| "only defined for real TimeData.", | ||
| ) |
There was a problem hiding this comment.
see note above on redundancy
| import re | ||
|
|
||
|
|
||
| def make_edc_from_energy(energy, sampling_rate=1000): |
There was a problem hiding this comment.
A copy of this function is already in the clarity pull. Lets discuss during the weekly how we want to handle this. In pyfar we have pyfar.testing, maybe we can do this here as well.
There was a problem hiding this comment.
That also applies to a few of the tests. Maybe a generalized approach would slim down the tests overall.
There was a problem hiding this comment.
If that is the case, we could think about joining the tests in test_parameters.py instead of having a separate file for each parameter. In this case the helper function could maybe also stay there.
There was a problem hiding this comment.
I opened an issue to keep track of this and would suggest to not fix it here or discuss in the weekly before fixing: #81
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
| definition = 1 - vals_energy_decay_curve / start_vals_energy_decay_curve | ||
| definition_db = 10 * np.log10(definition) |
There was a problem hiding this comment.
I feel like we should maybe rethink the implementation here a bit. Some of the energy based parameters can be calculated from others, see for example for definition from clarity here:
https://i-simpa-wiki.readthedocs.io/fr/latest/room_acoustics_parameters.html#definition
I would propose to take a step back and implement a general energy ratio calculation function first, as proposed by @HenningSchaar in #39 which could then be used in this function and the clarity calculation for example.
This function could also jointly take care of interval validity checking etc., so these do not need to be implemented in each function individually.
There was a problem hiding this comment.
Okay I like that idea of having this one unified energy-parameter function. At the same time calculating parameters from each other doesn't make that much sense if we have function as proposed in #39, right?
There was a problem hiding this comment.
+1 for a general energy ratio calculation function
github-project-automation
bot
moved this from Require review
to Done
in Weekly Planning
Changes proposed in this pull request: