Energy Ratio base function for room-acoustic parameters #108
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mberz
requested changes
Nov 28, 2025
Member
mberz
left a comment
mberz
left a comment
There was a problem hiding this comment.
Thanks for the new PR.
I've not read the docstring thoroughly, this is just a review of the code. The comment on the 2 EDCs is more a conceptual one.
What is the second edc for (it's not directly obvious, at least to me)? Calculation of strength?
github-project-automation
bot
moved this from Backlog
to Require review
in Weekly Planning
Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>
Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>
Docstring adjusted to explain neccessity of two separate EDCs
… correct cast of limits input from list
…rn result in cshape
hoyer-a
requested changes
Dec 1, 2025
Co-authored-by: Anton Hoyer <156099087+hoyer-a@users.noreply.github.com>
_energy_ratio edc input type pf.TimeData and derived objects changed Co-authored-by: Anton Hoyer <156099087+hoyer-a@users.noreply.github.com>
mberz
requested changes
Dec 2, 2025
Member
mberz
left a comment
mberz
left a comment
There was a problem hiding this comment.
A couple of minor things. After that it should be good to go. :)
Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com>
hoyer-a
reviewed
Dec 3, 2025
Member
hoyer-a
left a comment
hoyer-a
left a comment
There was a problem hiding this comment.
Thanks, the function looks good to me now!
Just some more comments on tests.
hoyer-a
requested changes
Dec 11, 2025
Member
hoyer-a
left a comment
hoyer-a
left a comment
There was a problem hiding this comment.
Thanks! The parametrization made one test redundant, apart from that it looks good to me.
mberz
approved these changes
Dec 12, 2025
Member
mberz
left a comment
mberz
left a comment
There was a problem hiding this comment.
Looks good, I'd like to squash the commit history though since it went through a lot of small changes during the review process.
github-project-automation
bot
moved this from Require review
to Done
in Weekly Planning
SimonBuechner
added a commit
that referenced
this pull request
Dec 12, 2025
* _energy_ratio and accordings tests added from corrupted branch * test adjusted to fixture, renamed to "...energy_ratio..." * test_parameters comment structure changed * Update pyrato/parameters.py balance -> ratio Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com> * Update pyrato/parameters.py, calculations more efficient Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com> * _energy_ratio accepts limits as ndarray, list and tuple. Docstring adjusted to explain neccessity of two separate EDCs * ruff compliancy added * test_energy_ratio_rejects_non_numpy_array_limits adjusted to check fo correct cast of limits input from list * order corrected in numerator-denominator extraction. Adjusted to return result in cshape * ruff compliancy, tests added to reject wrong limits type (new) * _energy_ratio input type documentation Co-authored-by: Anton Hoyer <156099087+hoyer-a@users.noreply.github.com> * Apply suggestions from code review _energy_ratio edc input type pf.TimeData and derived objects changed Co-authored-by: Anton Hoyer <156099087+hoyer-a@users.noreply.github.com> * _energy_ratio doc changed: requiring two edcs * ruff linting * _energy_ratio docs limit nomenclature Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com> * _energy_ratio docstring formulas changed * docstring examples added, edc-limit-idx separated each according edc * input check for limiti < signal_length of edc including tests added * valid limits tests for separate edcs included, tests modified accordingly * PEP8 compliancy * PEP8 compliancy * hoyer-a requested chanes in _energy_ratio tests * ruff linting * spelling changes * hoyer-a requested changes * renamed test * changed name test * adjusted parametrized test to multichannel input signal * redundant multichannel test removed * spelling in Docstring * re-added parametrized multichannel shape test --------- Co-authored-by: simon.buechner22 <simon.buechner22@campus.tu-berlin.de> Co-authored-by: Marco Berzborn <mberz@users.noreply.github.com> Co-authored-by: Anton Hoyer <156099087+hoyer-a@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue(s) are closed by this pull request?
Closes #39
Changes proposed in this pull request: