Added calculation of critical distance #102
Conversation
JudithvE
left a comment
JudithvE
left a comment
There was a problem hiding this comment.
Thanks a bunch!! We really appreciate you requesting a pull. All in all we are satisfied, however, we would like you to add an equation to the documentation. 👍
|
|
||
| return air_abs_coeff | ||
|
|
||
| def critical_distance( |
There was a problem hiding this comment.
| def critical_distance( | |
| def critical_distance(volume, reverberation_time): |
We would delete the enters here.
There was a problem hiding this comment.
Sorry we disagree, it is part of the pyfar coding guidelines to add these enters
There was a problem hiding this comment.
Both formatting with the new line and indent, as well as everything on one line are equally valid in pyfar ;)
pyrato/parametric.py
Outdated
| References | ||
| ---------- | ||
| .. [#] https://en.wikipedia.org/wiki/Critical_distance | ||
There was a problem hiding this comment.
Could you add the equation for the critical distance here?
pyrato/parametric.py
Outdated
| References | ||
| ---------- | ||
| .. [#] https://en.wikipedia.org/wiki/Critical_distance |
| """ | ||
| if reverberation_time <= 0: | ||
| raise ValueError("Reverberation time must be greater than zero.") |
ilsuono
left a comment
ilsuono
left a comment
There was a problem hiding this comment.
Thanks for contribs.
The functionalities are perfect, would be great if you can run ruff check and refer to CI checks to make sure the formattings are there and all the tests pass.
mjasins
left a comment
mjasins
left a comment
There was a problem hiding this comment.
Thank You, please include our minor comments.
| raise ValueError("Reverberation time must be greater than zero.") | ||
| if volume <= 0: | ||
| raise ValueError("Volume must be greater than zero.") | ||
| critical_dist = 0.057 * np.sqrt(volume / reverberation_time) |
There was a problem hiding this comment.
A minor comment: please name the return critical_distance to match pyfar convention
There was a problem hiding this comment.
Function name is also called critical_distance, so this would cause a conflict with this name.
There was a problem hiding this comment.
@TimSchellekensLAV Please consider renaming the function to calculate_critical_distance(), according to pyfar similar functions
There was a problem hiding this comment.
We are aware of this convention, though other functions in the parametric module do not follow this guideline either. To keep the module "coherent", we choose to join not following the guideline.
| reverberation_time | ||
| ): | ||
| """Calculate the critical distance of a room with given volume and reverberation time. | ||
There was a problem hiding this comment.
Please add the reference, as the link to the citation is not included and choose another source from Wikipedia :)
There was a problem hiding this comment.
Is this okay now? If yes, the conversation will be resolved.
ahms5
added
the
ASSA 2025
ahms5
left a comment
ahms5
left a comment
There was a problem hiding this comment.
Thank you for your contribution, looks great. I added a few suggestions.
pyrato/parametric.py
Outdated
| Assumes the source directivity is 1 (omnidirectional source). | ||
| .. math:: | ||
| d_c = 0.057 \\sqrt{\\frac{V}{T_{60}}} |
There was a problem hiding this comment.
eather use r""" as a raw docstring or use \ infront of special characters. Both together will not solve the problem
pyrato/parametric.py
Outdated
| r"""Calculate the critical distance of a room with | ||
| given volume and reverberation time [#kra]_. |
There was a problem hiding this comment.
please try to avoid using references in the first sentence of the summary. This will break the autoreferencing in the documentation.
| reverberation_time): | ||
| r"""Calculate the critical distance of a room with | ||
| given volume and reverberation time [#kra]_. | ||
| Assumes the source directivity is 1 (omnidirectional source). |
There was a problem hiding this comment.
I would be good practice, but not manorytory, to have a short summary in the first line and than more details to in a new paragraph like e.g. this:
"""
This is a short summary.
Give more details and even references here _[#].| """ | ||
| if reverberation_time <= 0: | ||
| raise ValueError("Reverberation time must be greater than zero.") |
JudithvE
left a comment
JudithvE
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
No remarks on this, looks good.
6 participants
Which issue(s) are closed by this pull request?
Closes issue #87
Changes proposed in this pull request:
-Added calculation critical distance based on volume and reverberation time