Refactor: Eyring reverberation time calculation#131
Conversation
- update input variable names - extend docstring - update tests
- formatting and linting fixes
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Eyring reverberation time calculation function to improve code quality and documentation. The changes rename parameters for clarity, add type hints, expand the docstring with mathematical formulas and examples, and make the function more flexible by adding a speed_of_sound parameter and supporting array inputs for mean_absorption.
Changes:
- Renamed function parameters:
surface→surface_area,mean_alpha→mean_absorption - Added type hints to function signature and new
speed_of_soundparameter with default value - Enhanced docstring with detailed mathematical formula, parameter descriptions, examples, and improved formatting
- Updated validation logic and calculation to use more explicit formula components
- Updated test file formatting (spacing, parametrize syntax) and error message matchers
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| pyrato/parametric.py | Refactored reverberation_time_eyring function with renamed parameters, type hints, expanded docstring, new speed_of_sound parameter, and updated calculation formula |
| tests/test_parametric_eyring.py | Updated test formatting, parametrize syntax, and error message matchers to align with refactored function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pyrato/parametric.py
Outdated
| def reverberation_time_eyring( | ||
| volume: float, | ||
| surface_area: float, | ||
| mean_absorption: np.typing.NDArray[float], |
There was a problem hiding this comment.
The type annotation np.typing.NDArray[float] is incorrect for the mean_absorption parameter. According to the docstring, this parameter can be either a float or a numpy.ndarray. The correct type annotation should be float | np.typing.NDArray[np.float64] or Union[float, np.typing.NDArray[np.float64]]. Additionally, np.typing.NDArray expects a numpy dtype, not the Python built-in float type.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mberz
added
enhancement
mberz
requested review from
a team,
SimonBuechner,
ahms5,
artur-pa,
f-brinkmann and
hoyer-a
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
Thanks - looks good to me.
github-project-automation
bot
moved this from Implementation in progress
to Reviewer Approved
in Weekly Planning
github-project-automation
bot
moved this from Reviewer Approved
to Done
in Weekly Planning
Changes proposed in this pull request:
I have refactored the reverberation time function based on Eyring's equation.