RAP center time#169
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds computation and test coverage for the room-acoustic center time parameter (T_s) based on an energy decay curve (EDC), integrating into the existing pyrato.parameters suite.
Changes:
- Add
center_time()topyrato/parameters.pyto compute (T_s) from an EDC. - Add unit tests for
center_time()intests/test_parameters.py. - Bump package version to
1.0.0in project metadata.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
pyrato/parameters.py |
Introduces center_time() parameter calculation from EDC data. |
tests/test_parameters.py |
Adds shape/type validation and analytical/multichannel tests for center_time(). |
pyrato/__init__.py |
Updates package __version__ to 1.0.0. |
pyproject.toml |
Updates project version and bumpversion current_version to 1.0.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
Thx, code and tests look fine. Can you merge the latest develop/main to get rid of the changes related to the version?
ahms5
left a comment
ahms5
left a comment
There was a problem hiding this comment.
thanks, looks fine, i rather have smaller suggestions.
| center_time(edc) | ||
|
|
||
| def test_center_time_rejects_zero_initial_energy(): | ||
| """Reject EDC with zero initial energy (would cause division by zero).""" |
There was a problem hiding this comment.
would it make sence to check for a plausible ETC as well, e.g. if the energy is positive and decaying, sth like np.all(np.diff(etc.time,axis=-1)<=0) and np.all(etc.time>=0)?
That might also be related to other methods and might be a larger thing.
There was a problem hiding this comment.
This issue actually involves several methods. Should we open a new pull request for this?
There was a problem hiding this comment.
The first check would typically fail for experimental data where the decay curve is not strictly exponential (decaying sinusoids)
There was a problem hiding this comment.
just decaying not exponential decaying.
Co-authored-by: Copilot <[email protected]>
artur-pa
force-pushed
the
feature/center_time
branch
from
8ac560a to
c12898b
Compare
…C due to truncation methods
mberz
left a comment
mberz
left a comment
There was a problem hiding this comment.
Thanks for implementing. Please see the comments below.
| center_time(edc) | ||
|
|
||
| def test_center_time_rejects_zero_initial_energy(): | ||
| """Reject EDC with zero initial energy (would cause division by zero).""" |
| center_time(edc) | ||
|
|
||
| def test_center_time_rejects_zero_initial_energy(): | ||
| """Reject EDC with zero initial energy (would cause division by zero).""" |
There was a problem hiding this comment.
just decaying not exponential decaying.
Changes proposed in this pull request:
center_time()function to parameters.py that computes the room-acoustic center timecenter_time()in test_parameters.py