Feature: add paris fomula to utils #35
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds the Paris formula implementation to calculate random-incidence coefficients based on Kuttruff's equation 2.53. The implementation provides a utility function for acoustic analysis with proper error handling and comprehensive test coverage.
- Implements
paris_formulafunction inimkar.utilsmodule with mathematical formula for random-incidence coefficient calculation - Adds comprehensive test suite covering various scenarios including constant/non-constant coefficients and error conditions
- Updates documentation structure to include the new utils module in API reference
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| imkar/utils.py | New module implementing the Paris formula with input validation and documentation |
| tests/test_utils.py | Comprehensive test suite covering function behavior and error cases |
| imkar/init.py | Exposes utils module in package interface |
| docs/modules/imkar.utils.rst | Documentation file for the utils module |
| docs/api_reference.rst | Updates API reference to include utils module |
Comments suppressed due to low confidence (1)
tests/test_utils.py:50
- This test expects an error message that mentions 'None or Coordinates', but passing None should be invalid based on the function logic. The test should either pass a valid non-Coordinates object or update the expected error message.
match="incident_directions have to be None or Coordinates"):
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
Thanks for the draft! Mostly had suggestions for documentation and improved robustness.
| Calculate the random-incidence coefficient | ||
| according to Paris formula. | ||
|
|
||
| The implementation follows the Equation 2.53 from [#]_ and is |
There was a problem hiding this comment.
Can you please add a little general introduction? Something like: 'The Paris formula computes the ... It is valid for... and requires ...'. This is valid for scattering, absorption and generally all direction dependend energetic properties?
imkar/utils.py
Outdated
| coefficients : pyfar.FrequencyData | ||
| coefficients for different incident directions. Its cshape | ||
| needs to be (..., n_incident_directions) | ||
| incident_directions : pyfar.Coordinates |
There was a problem hiding this comment.
SamplingSphere would have the check for equal radii built in, but it has a 4 pi restriction on the sum of the weights, which we probably do not want here. Check for equal radius and add radius tolerance in this function as well?
imkar/utils.py
Outdated
| if not isinstance(coefficients, pf.FrequencyData): | ||
| raise ValueError("coefficients has to be FrequencyData") | ||
| if not isinstance(incident_directions, pf.Coordinates): |
There was a problem hiding this comment.
using e.g. type(coefficients) != pf.FrequencyData is more future-safe because isinstance evaluates to True also for inherited classes.
| with the ``coefficients`` :math:`c`, and the | ||
| area weights :math:`w` from the ``incident_directions``. | ||
| :math:`|\Omega_S \cdot n|` represent the cosine of the angle between the | ||
| surface normal and the incident direction. |
There was a problem hiding this comment.
This is very implicit. Can you please be more verbose here and also add a check for np.all(incident_directions.z >= 0?
| (0), (0.2), (0.5), (0.8), (1)]) | ||
| @pytest.mark.parametrize("frequencies", [ | ||
| ([100, 200]), ([100])]) | ||
| def test_random_constant_coefficient( |
There was a problem hiding this comment.
A little docstring or comments for this test would be nice.
| assert c_rand.comment == 'random-incidence coefficient' | ||
|
|
||
|
|
||
| def test_random_non_constant_coefficient(): |
There was a problem hiding this comment.
docstring and/or comments would be nice.
| (0), (0.2), (0.5), (0.8), (1)]) | ||
| @pytest.mark.parametrize("frequencies", [ | ||
| ([100, 200]), ([100])]) | ||
| def test_random_constant_coefficient( |
There was a problem hiding this comment.
I would suggest that all tests start with test_paris_formula_ in case more functios are added to the module later.
Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>
Changes proposed in this pull request: