-
Notifications
You must be signed in to change notification settings - Fork 15
Adding Eyring's equation #104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Adding Eyring's equation #104
Conversation
mjasins
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You for Your contribution. Please, look at our minor comment changes.
pyrato/parametric.py
Outdated
|
|
||
| def Eyrings_equation(volume,surface,mean_alpha): | ||
| """ | ||
| function which calculates reverberation time in rooms as defined by Carl F. Eyring. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, please change the function name according to project convention, eg. calculate_eyring_reverberation_time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I added a reference. I will keep the name as it is similar to the other names in parametric.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank You - the request to change the function name was my fault - sorry for that.
pyrato/parametric.py
Outdated
| Parameters | ||
| ---------- | ||
| volume : float, np.ndarray | ||
| Room volume |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, specify units
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I added units
pyrato/parametric.py
Outdated
| volume : float, np.ndarray | ||
| Room volume | ||
| sufaces : float, np.ndarray | ||
| Surface areas of all surfaces in the room |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, specify units
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I added a unit
pyrato/parametric.py
Outdated
| Average absorption coefficient of room surfaces | ||
| Returns | ||
| ------- | ||
| reverb_eyring: double |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return in docstring does not match the actual return. Please consider and rename it to reverberation_time_eyring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the proposal, I have used reverberation_time_eyring now
mberz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request.
I have some comments on input checking and equation rendering.
pyrato/parametric.py
Outdated
| .. math:: | ||
| T_{60} = -0.161 \frac{\text{volume}}{\text{surface} \cdot \ln(1 - \text{mean\_alpha})} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .. math:: | |
| T_{60} = -0.161 \frac{\text{volume}}{\text{surface} \cdot \ln(1 - \text{mean\_alpha})} | |
| .. math:: | |
| T_{60} = -0.161 \frac{\text{volume}}{\text{surface} \cdot \ln(1 - \text{mean\_alpha})} | |
I think this could solve the formatting issue on the rendered docs.
| .. [#] Eyring, C.F., 1930. Reverberation time in “dead” rooms. The Journal of the Acoustical Society of America, 1(2A_Supplement), pp.168-168. | ||
| """ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could add some input data checks for cases where the equation is not giving valid results or where the computation might crash, such as negative surface areas or volumes.
Which issue(s) are closed by this pull request?
Closes #85
Changes proposed in this pull request:
An equation is added that calculates the reverberation time based on the Equation as proposed by Carl. F. Eyring