feature/add_schroeder_frequency_function#101
Conversation
… the extra delicous schroeder frequency.
mjasins
left a comment
mjasins
left a comment
There was a problem hiding this comment.
Thank You for Your brilliant contribution. We have spotted some minor changes, that You should make to match the project convention.
pyrato/parametric.py
Outdated
|
|
||
| Parameters | ||
| ---------- | ||
| V : float, np.ndarray |
There was a problem hiding this comment.
Please follow the variable naming convention adopted in the project, i.e.:
volume (double) – Room volume
reverberation_time (double) - Reverberation Time of a room
pyrato/parametric.py
Outdated
| function which calculates the Schroeder frequency. The cut-off | ||
| frequency for modes. | ||
|
|
||
| Approved by Monty Python Flying Circus. |
There was a problem hiding this comment.
We appreciate Your sense of humour, although this should be a serious scientific documentation.
pyrato/parametric.py
Outdated
|
|
||
| def schroeder_frequency(V, T): | ||
| r""" | ||
| function which calculates the Schroeder frequency. The cut-off |
There was a problem hiding this comment.
Please, change the sentence to: Calculate the Schroeder cut-off frequency of a room
| r""" | ||
| function which calculates the Schroeder frequency. The cut-off | ||
| frequency for modes. | ||
|
|
There was a problem hiding this comment.
Please add the source - the citation is broken
There was a problem hiding this comment.
nice hint, but we don´t know how ... seems as broken as for energy_decay_curve_analytic function below.
pyrato/parametric.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| schroeder_freq : float, np.ndarray |
There was a problem hiding this comment.
The documentation return statement does not match the actual return variable name. Please repair and change to schroeder_frequency according to convention
|
Hey, thanks for your contribution. Could you please update the title of the PR to be more concise? |
SitChri
changed the title
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. I already looks very good.
I have just one suggestion to raise errors if the input parameters are not as expected
| .. [#] H. Kuttruff, Room acoustics, 4th Ed. Taylor & Francis, 2009. | ||
|
|
||
| """ | ||
|
|
There was a problem hiding this comment.
here we could raise error, if the input parameters of the function are not valid. See the Guidelines
JudithvE
left a comment
JudithvE
left a comment
There was a problem hiding this comment.
Thank you for your kind pull request. We could not have done this better. 👍
Don't forget to add a validation check for the input parameters 😄
| .. [#] H. Kuttruff, Room acoustics, 4th Ed. Taylor & Francis, 2009. | ||
|
|
||
| """ | ||
|
|
There was a problem hiding this comment.
Maybe you can do a validation check of the input parameters. For example, the volume needs to be greater than zero. 🔢
mjasins
left a comment
mjasins
left a comment
There was a problem hiding this comment.
Thank You for the submission, the tests are really good.
5 participants
Which issue(s) are closed by this pull request?
Closes #88. implementation of Schroeder frequency....
Changes proposed in this pull request: