Feature: correlation method after mommertz #31

ahms5 · 2025-04-25T10:11:06Z

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

Closes #12

Changes proposed in this pull request:

introduce correlation method after mommertz, in other words, calcualte scattering coefficient from freefield measurements depending on the incident direciton

requires #32

imkar/scattering/freefield.py

f-brinkmann

Thanks for the feature. It generally looks good to me. I had mostly minor comments.

f-brinkmann · 2025-07-25T13:04:03Z

imkar/scattering/freefield.py

+        - :math:`w` represents the area weights of the sampling, and
+          :math:`\vartheta` and :math:`\varphi` are the ``colatitude``
+          and ``azimuth`` angles from the
+          :py:class:`~pyfar.classes.coordinates.Coordinates` object.


I would rather link here than to the coordinates object for explaining the angles: https://pyfar.readthedocs.io/en/stable/classes/pyfar.coordinates.html#coordinate-systems. And I think it would help to mention that they denote the incidence direction

f-brinkmann · 2025-07-25T13:08:44Z

imkar/scattering/freefield.py

+    Returns
+    -------
+    scattering_coefficients : :py:class:`~pyfar.classes.audio.FrequencyData`
+        The scattering coefficient for each incident direction as a function


Maybe mention here and also above, that the channels up to the last channel dimension of sample_pressure, reference_pressure and scattering_coefficients represent the incident direction. It might help to get an example as 'cshape = (45, 25) denotes a measurement with 45 incidenet directions and recorded at 25 microphone positions.'

actually it is just relevant what the last cdim is, because this is the receiver grid, from which we are averaging. what ever is before that (typically incident direction) will come out. it can also be just one incident direction. I tired to make it more clear in th docstring

f-brinkmann · 2025-07-25T13:11:46Z

imkar/scattering/freefield.py

+          and ``azimuth`` angles from the
+          :py:class:`~pyfar.classes.coordinates.Coordinates` object.
+
+    The test sample is assumed to lie in the x-y-plane.


Is this relevant for the computation? Otherwise this information could be omitted.

f-brinkmann · 2025-07-25T13:13:25Z

imkar/scattering/freefield.py

+    if not isinstance(sample_pressure, pf.FrequencyData):
+        raise TypeError("sample_pressure must be of type pyfar.FrequencyData")
+    if not isinstance(reference_pressure, pf.FrequencyData):
+        raise TypeError(
+            "reference_pressure must be of type pyfar.FrequencyData")


This will pass also for signals. You have to use type(sample_pressure) == pf.FrequencyData

yes it can also be an impulse response, where we just use freq data. I made it more clear in docs and checks

f-brinkmann · 2025-07-25T13:15:31Z

imkar/scattering/freefield.py

+    if sample_pressure.cshape[:-1] != reference_pressure.cshape[:-1]:
+        raise ValueError(
+            "The cshape of sample_pressure and reference_pressure must be "
+            "broadcastable except for the last dimension")


I think this error can not be reached and should be deleted. One of the two if-cases above would already if sample_pressure.cshape[:-1] != reference_pressure.cshape[:-1]

f-brinkmann · 2025-07-25T13:24:11Z

tests/test_scattering_freefield.py

+from imkar.scattering import freefield as sff
+
+
+def plane_wave(amplitude, direction, sampling):


Please add a minimal docstring to ease maintainance

f-brinkmann · 2025-07-25T13:25:24Z

tests/test_scattering_freefield.py

+
+
+def test_correlation_method_0():
+    sampling = pf.samplings.sph_equal_area(5000)


Better already use samplings from spharpy?

I'll shift it to the todo, after the spharpy release

f-brinkmann · 2025-07-25T13:28:20Z

tests/test_scattering_freefield.py

+
+@pytest.mark.parametrize("s_scatter", [0.1, 0.5, 0.9])
+@pytest.mark.parametrize("Phi_scatter_deg", [30, 60, 90, 120, 150, 42])
+def test_correlation_method_with_plane_waves(s_scatter, Phi_scatter_deg):


This would take a little while to understand. Can you add some comments for guidance?

f-brinkmann · 2025-07-25T13:29:33Z

tests/test_scattering_freefield.py

+    )
+
+
+def test_correlation_method_0():


Could this be integrated into test_correlation_method_with_plane_waves?

f-brinkmann · 2025-07-25T13:29:54Z

tests/test_scattering_freefield.py

+    npt.assert_almost_equal(sd_scatter_0.freq, 0, 1)
+
+
+def test_correlation_method():


In how far is this different from test_correlation_method_0 above?

Copilot

Pull Request Overview

This PR introduces a new correlation method for calculating scattering coefficients from free-field measurements based on the Mommertz correlation method. The implementation allows users to compute scattering coefficients by analyzing reflection patterns from samples compared to reference measurements.

Key changes:

Implements the Mommertz correlation method for scattering coefficient calculation
Adds comprehensive test coverage for various scattering scenarios
Updates module structure and documentation to include the new freefield scattering functionality

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
imkar/scattering/freefield.py	Core implementation of the correlation_method function with mathematical formulation and input validation
tests/test_scattering_freefield.py	Comprehensive test suite covering edge cases, parametric testing, and input validation
imkar/scattering/init.py	New module initialization file exposing freefield submodule
imkar/init.py	Updated package initialization to include scattering module
docs/modules/imkar.scattering.freefield.rst	Documentation file for the new freefield module
docs/modules/imkar.rst	Removed original imkar module documentation
docs/api_reference.rst	Updated API reference to point to new freefield module

Comments suppressed due to low confidence (2)

tests/test_scattering_freefield.py:130

The error message in the test doesn't match the actual error message from the function. The function accepts both FrequencyData and Signal types, but the test expects an error message mentioning only FrequencyData.

            TypeError, match="sample_pressure must be of type "

tests/test_scattering_freefield.py:140

The error message in the test doesn't match the actual error message from the function. The function accepts both FrequencyData and Signal types, but the test expects an error message mentioning only FrequencyData.

            TypeError, match="reference_pressure must be of type "

imkar/scattering/freefield.py

tests/test_scattering_freefield.py

Co-authored-by: Copilot <[email protected]>

sikersten

Looks good, only minor things. I'm not sure if I understood the testing in detail though...

imkar/scattering/freefield.py

sikersten · 2025-08-21T13:26:20Z

imkar/scattering/freefield.py

+        cshape of ``reference_pressure``. The frequency vectors of both
+        ``sample_pressure`` and ``reference_pressure`` must match.
+    reference_pressure : pyfar.FrequencyData, pyfar.Signal
+        Reflected sound pressure or directivity of the reference sample. Its


What is meant with the "or"? Do reflected sound pressure and directivity mean the same thing or can both be used as input for the function?

both can be used, because it is proportional

imkar/scattering/freefield.py

sikersten · 2025-08-21T13:32:02Z

imkar/scattering/freefield.py

+            "broadcastable except for the last dimension")
+    # Test whether the objects are able to perform arithmetic operations.
+    # e.g. does the frequency vectors match
+    _ = sample_pressure + reference_pressure


Is checking the frequencies the only purpose? If yes, +1 for an explicit solution

sikersten · 2025-08-21T13:40:44Z

imkar/scattering/freefield.py

+    p_sample_sum = np.sum(microphone_weights * np.abs(p_sample)**2, axis=-2)
+    p_ref_sum = np.sum(microphone_weights * np.abs(p_reference)**2, axis=-2)
+    p_cross_sum = np.sum(
+            p_sample * np.conj(p_reference) * microphone_weights, axis=-2)
+
+    data_scattering_coefficient = 1 - np.abs(p_cross_sum)**2/(
+        p_sample_sum*p_ref_sum)


It would be cool to have pyfar functions sum, abs, conj in the future, wouldn't it? pyfar/pyfar#830

tests/test_scattering_freefield.py

sikersten · 2025-08-21T13:58:25Z

tests/test_scattering_freefield.py

+    npt.assert_almost_equal(sd_scatter_0.freq, 0, 1)
+
+
+def test_correlation_one_scattering():


Short docstrings would be helpful :)

Co-authored-by: sikersten <[email protected]>

draft correlation method after mommertz

f484231

ahms5 added this to the v0.1.0 milestone Apr 25, 2025

ahms5 added this to Weekly Planning Apr 25, 2025

github-project-automation bot moved this to Backlog in Weekly Planning Apr 25, 2025

ahms5 added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 25, 2025

ahms5 self-assigned this Apr 25, 2025

ahms5 added 2 commits April 25, 2025 12:18

Merge branch 'develop' into scattering/correlation_method

6ac7a4d

remove unused files

a865d2c

ahms5 moved this from Backlog to Require review in Weekly Planning May 9, 2025

ahms5 commented May 26, 2025

View reviewed changes

imkar/scattering/freefield.py Show resolved Hide resolved

ahms5 and others added 2 commits May 27, 2025 01:07

Apply suggestions from code review

ae1a7c3

fix tests

026f804

ahms5 changed the title ~~draft correlation method after mommertz~~ Feature: correlation method after mommertz May 26, 2025

ahms5 requested a review from Copilot May 26, 2025 23:49

This comment was marked as outdated.

Sign in to view

ahms5 requested review from f-brinkmann, hoyer-a, mberz and sikersten May 27, 2025 07:52

f-brinkmann requested changes Jul 25, 2025

View reviewed changes

apply review, enhance tests and tests doc

fba1ada

ahms5 requested review from Copilot and f-brinkmann July 29, 2025 08:32

fix ruff

67de445

This comment was marked as outdated.

Sign in to view

ahms5 added 2 commits July 29, 2025 10:35

improve docstring

051fb26

fix tests

205f481

ahms5 mentioned this pull request Jul 29, 2025

New scattering freefield implementation #12

Closed

make pyfar.FrequencyData link to pyfar doc

0e7714a

ahms5 requested a review from Copilot July 29, 2025 10:09

Copilot AI reviewed Jul 29, 2025

View reviewed changes

imkar/scattering/freefield.py Show resolved Hide resolved

tests/test_scattering_freefield.py Outdated Show resolved Hide resolved

tests/test_scattering_freefield.py Outdated Show resolved Hide resolved

tests/test_scattering_freefield.py Outdated Show resolved Hide resolved

Apply suggestions from code review

06581ba

Co-authored-by: Copilot <[email protected]>

sikersten reviewed Aug 21, 2025

View reviewed changes

ahms5 and others added 2 commits August 22, 2025 10:39

Apply suggestions from code review

51e459b

Co-authored-by: sikersten <[email protected]>

apply review

232bc97

ahms5 requested a review from sikersten August 22, 2025 09:36

		from imkar.scattering import freefield as sff


		def plane_wave(amplitude, direction, sampling):



		def test_correlation_method_0():
		sampling = pf.samplings.sph_equal_area(5000)

		npt.assert_almost_equal(sd_scatter_0.freq, 0, 1)


		def test_correlation_method():

		npt.assert_almost_equal(sd_scatter_0.freq, 0, 1)


		def test_correlation_one_scattering():

Feature: correlation method after mommertz #31

Are you sure you want to change the base?

Feature: correlation method after mommertz #31

Uh oh!

Conversation

ahms5 commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Changes proposed in this pull request:

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sikersten left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahms5 commented Apr 25, 2025 •

edited

Loading