Skip to content

Conversation

@ahms5
Copy link
Member

@ahms5 ahms5 commented Jul 29, 2025

Which issue(s) are closed by this pull request?

Closes #28

Changes proposed in this pull request:

  • maximum_sample_absorption_coefficient(frequencies)->FrequencyData (returns 0.5 for the given freqeunceis)
  • maximum_baseplate_scattering_coefficient(N)->FrequencyData (the table 1)

@ahms5 ahms5 added this to the v0.1.0 milestone Jul 29, 2025
@ahms5 ahms5 requested a review from Copilot July 29, 2025 09:19
@ahms5 ahms5 self-assigned this Jul 29, 2025
@ahms5 ahms5 added the enhancement New feature or request label Jul 29, 2025
@ahms5 ahms5 changed the base branch from main to develop July 29, 2025 09:19

This comment was marked as outdated.

@ahms5 ahms5 changed the title Feature: Scattering calculation ISO Feature: Scattering ISO limits Jul 29, 2025
@ahms5 ahms5 mentioned this pull request Jul 29, 2025
@ahms5 ahms5 requested a review from Copilot July 29, 2025 10:09
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements ISO 17497-1:2004 scattering limits by adding two key functions for diffuse scattering calculations in acoustics. The implementation provides standardized maximum values for absorption and scattering coefficients used in reverberation room measurements.

  • Adds maximum_sample_absorption_coefficient() function that returns a constant value of 0.5 for given frequencies per ISO standards
  • Adds maximum_baseplate_scattering_coefficient() function that implements Table 1 from ISO 17497-1:2004 with optional scale factor support
  • Creates comprehensive test suite covering input validation, data integrity, and edge cases

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
imkar/scattering/diffuse.py Core implementation of the two ISO limit functions with input validation and pyfar integration
tests/test_scattering_diffuse.py Comprehensive test suite covering normal operation, edge cases, and error conditions
imkar/scattering/__init__.py Module initialization exposing the diffuse submodule
imkar/__init__.py Package-level import of the scattering module
docs/modules/imkar.scattering.diffuse.rst Sphinx documentation configuration for the new module
docs/modules/imkar.rst Removed old module documentation file
docs/api_reference.rst Updated API reference to point to the new diffuse module
Comments suppressed due to low confidence (2)

tests/test_scattering_diffuse.py:68

  • The test function name suggests it tests negative N values, but the test uses N=5 which is positive. The name should be updated to reflect what the test actually does, such as 'test_maximum_baseplate_scattering_coefficient_positive_scale_factor'.
def test_maximum_baseplate_scattering_coefficient_negative_N():

tests/test_scattering_diffuse.py:64

  • The test checks TypeError for float input but doesn't test the zero case. Consider adding a test for N=0 to ensure the validation properly handles this edge case.
        isd.maximum_baseplate_scattering_coefficient(N=1.5)

@ahms5 ahms5 moved this from Backlog to Require review in Weekly Planning Jul 29, 2025
Copy link
Member

@hoyer-a hoyer-a left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just one thing.

with pytest.raises(TypeError, match="N must be a positive integer."):
isd.maximum_baseplate_scattering_coefficient(N=-1)

def test_maximum_baseplate_scattering_coefficient_negative_N():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be _positive_N?

@ahms5 ahms5 requested a review from hoyer-a July 31, 2025 11:01
@github-project-automation github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Jul 31, 2025
@ahms5 ahms5 moved this from Reviewer Approved to Require review in Weekly Planning Jul 31, 2025
@ahms5 ahms5 requested a review from a team October 8, 2025 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

Status: Require review

Development

Successfully merging this pull request may close these issues.

scattering calculation ISO 17497-1:2004

3 participants