New scattering freefield implementation #12

ahms5 · 2023-02-24T16:55:47Z

Changes proposed in this pull request:

implement scattering coefficient calculation from freefield measurements/simulations

replaces #8

This reverts commit bcb4fbc.

imkar/scattering/coefficient.py

sikersten · 2023-03-20T13:39:33Z

imkar/scattering/coefficient.py

+        Defines the incidence directions of each `scattering_coefficients` in a
+        Coordinates object. Its cshape need to be (#angle1, #angle2). In
+        sperical coordinates the radii  need to be constant.


According to the code, this is a little confusing. Do I understand it correctly that it needs to be a spherical sampling incl. weights? Why shaped to (#angle1, #angle2)? What is angle1 and angle2?

its wrong, its a leftover, thanks

imkar/scattering/coefficient.py

sikersten · 2023-03-20T13:44:03Z

imkar/scattering/coefficient.py

+        raise ValueError("incident_positions have to be None or Coordinates")
+
+    theta = incident_positions.get_sph().T[1]
+    weight = np.cos(theta) * incident_positions.weights


How can it be ensured, that the incident_positions.weights are meaningful?

added in the docs

sikersten · 2023-03-20T13:48:46Z

tests/test_scattering_coefficient.py

+def test_freefield_1(frequencies):
+    mics = pf.samplings.sph_gaussian(42)
+    mics = mics[mics.get_sph().T[1] <= np.pi/2]  # delete lower part of sphere
+    p_sample = stub_utils.frequency_data_from_shape(


According to the pyfar "rules", this is not how stub utils should be used. Please create the stubs in conftest.py, not within the tests itself. Maybe the stub_utils are even obsolete then (which would be favorable)?

Co-authored-by: sikersten <70263411+sikersten@users.noreply.github.com>

ahms5 · 2023-04-16T19:05:09Z

ready to review

pingelit

Looks good to me. Except for some minor doc string change, I would approve.

pingelit · 2023-05-11T12:38:32Z

imkar/scattering/scattering.py

+            "same cshape.")
+    if microphone_weights.shape[0] != sample_pressure.cshape[-1]:
+        raise ValueError(
+            "the last dimension of sample_pressure need be same as the "


Suggested change

"the last dimension of sample_pressure need be same as the "

"the last dimension of sample_pressure needs to be the be same as the "

imkar/utils/utils.py

pingelit · 2023-05-11T12:50:16Z

imkar/utils/utils.py

+        need to be (..., #incident_directions)
+    incident_directions : pyfar.Coordinates
+        Defines the incidence directions of each `coefficients` in a
+        Coordinates object. Its cshape need to be (#incident_directions). In


Suggested change

Coordinates object. Its cshape need to be (#incident_directions). In

Coordinates object. Its cshape needs to be (#incident_directions). In

pingelit · 2023-05-11T12:51:14Z

imkar/utils/utils.py

+    Returns
+    -------
+    random_coefficient : pyfar.FrequencyData
+        The random-incidence scattering coefficient.


I don't know how common the Paris formula is, but it might be nice to add a citation here as well.

Co-authored-by: Pascal Palenda <pingelit@users.noreply.github.com>

ahms5 · 2023-05-23T22:43:00Z

ready for review

f-brinkmann · 2023-06-02T12:08:52Z

docs/modules/imkar.scattering.rst

@@ -0,0 +1,7 @@
+imkar.scattering
+================
+


A brief general description of the module in a sentence or two might be nice to add.

f-brinkmann · 2023-06-02T12:09:31Z

imkar/scattering/scattering.py

+import numpy as np
+import pyfar as pf


See issue - dependencies must be added to setup.py

f-brinkmann · 2023-06-02T12:12:12Z

imkar/scattering/scattering.py

+    Calculate the free-field scattering coefficient for each incident direction
+    using the Mommertz correlation method [1]_:


Should we go full Zen of Python and start with a one liner without reference? I would also not use incident direction, since this is not passed and seems to be implicitly contained in the input data.

Suggested change

Calculate the free-field scattering coefficient for each incident direction

using the Mommertz correlation method [1]_:

Calculate the direction dependent free-field scattering coefficient.

Uses the Mommertz correlation method [1]_ to calculate the scattering

coefficient separately for each channel of the input data:

f-brinkmann · 2023-06-02T12:14:51Z

imkar/scattering/scattering.py

+
+    .. math::
+        s(\vartheta_S,\varphi_S) = 1 -
+            \frac{|\sum \underline{p}_{sample}(\vartheta_R,\varphi_R) \cdot


For readability I would suggest to use text in the subscripts, e.g.: p_{\text{sample}}

f-brinkmann · 2023-06-02T12:17:13Z

imkar/scattering/scattering.py

+    with the ``sample_pressure``, the ``reference_pressure``, and the
+    area weights ``weights_microphones``. See


I would focus more on explaining the formula, e.g., with the sample pressure p_{\text{sample}} and not refer to the variable names here. To me that was clear due to the verbose variable names already.

Can we use \sum_w and w(phi_R, theta_R) to make things more clear?

f-brinkmann · 2023-06-02T12:53:10Z

imkar/scattering/scattering.py

+        Coordinates object. Its cshape needs to be (#source_directions). In
+        sperical coordinates the radii needs to be constant. The weights need
+        to reflect the area weights.


A cross reference to pyfar.Coordinates would be convenient.

'Points contained in incident_directions must hace the same radii' would sound more clear to me.

I would suggest to explicitly refer to incident_directions.weights to be more clear

f-brinkmann · 2023-06-02T12:53:37Z

imkar/scattering/scattering.py

+    """
+    random_scattering = utils.paris_formula(
+        scattering_coefficients, incident_directions)
+    random_scattering.comment = 'random-incidence scattering coefficient'


See above for question about using/not using comments

f-brinkmann · 2023-06-02T12:55:12Z

imkar/utils/utils.py

+    Calculate the random-incidence coefficient
+    according to Paris formula [2]_.
+
+    .. math::
+        c_{rand} = \sum c(\vartheta_S,\varphi_S) \cdot cos(\vartheta_S) \cdot w
+
+    with the ``coefficients``, and the
+    area weights ``w`` from the ``incident_directions``.
+    Note that the incident directions should be
+    equally distributed to get a valid result.


See general docstring comments in scattering.random

f-brinkmann · 2023-06-02T12:57:04Z

imkar/scattering/scattering.py

+    ----------
+    sample_pressure : pyfar.FrequencyData
+        Reflected sound pressure or directivity of the test sample. Its cshape
+        needs to be (..., #microphones).


'#microphones' is a bit informal. Should we use variable_name.size? Would also affect the remaining docstrings.

f-brinkmann · 2023-06-02T12:57:29Z

imkar/utils/utils.py

+        Defines the incidence directions of each `coefficients` in a
+        Coordinates object. Its cshape needs to be (#incident_directions). In
+        sperical coordinates the radii needs to be constant. The weights need
+        to reflect the area weights.


See comment in scattering.random

f-brinkmann

Thanks for the effort. I had one big questions:

Should we make the functions work with pyfar.Signal objects. I think yes - but what would be a clean, simple, and comprehensive way? This would require filtering in octaves and maybe in som cases different processing in the functions

In the docstrings

it would be safer to number the references implicitly (I think using [#]_ , there should be examples in the pyfar code base)
parameters should be written in italic unsing only a single `

f-brinkmann

Thanks for the effort. Only had a few comments left and I was wondering if the public utils model should be added to the docs.

imkar/scattering/scattering.py

f-brinkmann · 2023-07-28T07:18:13Z

imkar/scattering/scattering.py

+        Reflected sound pressure or directivity of the
+        reference sample. Needs to have the same cshape and frequencies as
+        `sample_pressure`.
+    microphone_weights : numpy.ndarray


Are there constraints on the weights that should be mentioned? E.g., should they sum to 1?

no need to be normalized or just area weights, but mybe i could mention it

imkar/scattering/scattering.py

f-brinkmann · 2023-07-28T07:23:14Z

imkar/utils/utils.py

+import pyfar as pf
+
+
+def paris_formula(coefficients, incident_directions):


The name of the function and module suggests that it is public. In this case should we add it to the docs as well?

imkar/utils/utils.py

f-brinkmann · 2023-07-28T07:24:36Z

imkar/utils/utils.py

+    and :math:`\vartheta` and :math:`\varphi` are the incidence
+    angle and azimuth angles. Note that the incident directions should be


the term 'incedence angle and azimuth anges' is a bit confusing to me. Could we use only 'incidence angles'?

solved it, by directly refering to the pyfar.Coordinates angles

f-brinkmann · 2023-07-28T07:25:19Z

imkar/scattering/scattering.py

+    the reference sample (same dimension as the sample under investigation,
+    but with flat surface) :math:`\underline{p}_{\text{reference}}`, the
+    area weights of the sampling :math:`w`, and :math:`\vartheta` and
+    :math:`\varphi` are the incidence angle and azimuth angles. See


See comment in utils on the names of the angles.

f-brinkmann · 2023-07-28T07:26:53Z

imkar/scattering/scattering.py

+        needs to be (..., microphone_weights.csize).
+    reference_pressure : :py:class:`~pyfar.classes.audio.FrequencyData`
+        Reflected sound pressure or directivity of the
+        reference sample. Needs to have the same cshape and frequencies as


Should we link to pyfar audio concepts when refering to cshape? Otherwise people less familiar with pyfar might be confused.

Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>

This reverts commit ffaad46, reversing changes made to 4f98a5b.

impl new approach for scattering

a283387

ahms5 mentioned this pull request Feb 24, 2023

Scattering-freefield #8

Closed

ahms5 added 4 commits February 24, 2023 18:49

make diffusion module available

bcb4fbc

Revert "make diffusion module available"

38eefd7

This reverts commit bcb4fbc.

add formula to doc

a88b34f

fix doc

5b8a350

ahms5 requested review from mberz, f-brinkmann and sikersten March 10, 2023 14:04

ahms5 self-assigned this Mar 10, 2023

ahms5 added 2 commits March 10, 2023 15:32

add paris formula to docs

ac207bc

fix docs

800b72c

sikersten requested changes Mar 20, 2023

View reviewed changes

ahms5 and others added 8 commits March 24, 2023 12:27

Apply suggestions from code review

2227ef4

Co-authored-by: sikersten <70263411+sikersten@users.noreply.github.com>

new structure

47802e4

better labeling

112cf90

fix new structure in docs

0baf823

should not be part of this pr

00f76b1

remove stubs as proposed by @sikersten

b29353f

separate paris formula in utils as discussed

5c7408e

flake8

0987a4a

ahms5 requested a review from sikersten April 16, 2023 19:04

ahms5 requested a review from pingelit April 28, 2023 11:16

pingelit requested changes May 11, 2023

View reviewed changes

ahms5 and others added 4 commits May 23, 2023 17:58

Update imkar/utils/utils.py

bb33688

Co-authored-by: Pascal Palenda <pingelit@users.noreply.github.com>

Update imkar/utils/utils.py

d0544c9

Co-authored-by: Pascal Palenda <pingelit@users.noreply.github.com>

add comments @pingelit

87c48bf

cosmetics

ada05f8

ahms5 requested a review from pingelit May 23, 2023 22:42

f-brinkmann requested changes Jun 2, 2023

View reviewed changes

pingelit approved these changes Jun 9, 2023

View reviewed changes

pingelit mentioned this pull request Jun 9, 2023

New diffusion coefficient #14

Open

ahms5 added 6 commits June 12, 2023 09:38

add review @f-brinkmann

df55753

try to add reference in doc to pyfar

4d892eb

cosmetics + integrate last review comments

f993b39

cosmetics + integrate last review comments

02ef6db

fix tests

8d3030e

fix doc

e005c98

ahms5 requested a review from f-brinkmann June 26, 2023 08:48

f-brinkmann requested changes Jul 28, 2023

View reviewed changes

ahms5 and others added 4 commits August 7, 2023 13:40

Update imkar/scattering/scattering.py

ac8de6f

Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>

Update imkar/scattering/scattering.py

08d0b0f

Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>

Update imkar/utils/utils.py

ae4140d

Co-authored-by: Fabian Brinkmann <fabian.brinkmann@mailbox.org>

apply review @f-brinkmann

4f98a5b

ahms5 requested a review from f-brinkmann August 7, 2023 11:57

ahms5 added 4 commits February 20, 2024 16:32

apply cookiecutter

599cd70

fix flake8

723c48c

Merge branch 'cookiecutter' into new_scattering_freefield

ffaad46

Revert "Merge branch 'cookiecutter' into new_scattering_freefield"

08504b3

This reverts commit ffaad46, reversing changes made to 4f98a5b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New scattering freefield implementation #12

New scattering freefield implementation #12

ahms5 commented Feb 24, 2023

sikersten Mar 20, 2023

ahms5 Mar 24, 2023

sikersten Mar 20, 2023

ahms5 Mar 24, 2023

sikersten Mar 20, 2023

ahms5 Apr 16, 2023

ahms5 commented Apr 16, 2023

pingelit left a comment

pingelit May 11, 2023

pingelit May 11, 2023

pingelit May 11, 2023

ahms5 commented May 23, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann Jun 2, 2023

f-brinkmann left a comment

f-brinkmann left a comment

f-brinkmann Jul 28, 2023

ahms5 Jul 28, 2023

f-brinkmann Jul 28, 2023

f-brinkmann Jul 28, 2023

ahms5 Aug 7, 2023

f-brinkmann Jul 28, 2023

f-brinkmann Jul 28, 2023

	"the last dimension of sample_pressure need be same as the "
	"the last dimension of sample_pressure needs to be the be same as the "

	Coordinates object. Its cshape need to be (#incident_directions). In
	Coordinates object. Its cshape needs to be (#incident_directions). In

		Calculate the free-field scattering coefficient for each incident direction
		using the Mommertz correlation method [1]_:

-    Calculate the free-field scattering coefficient for each incident direction
-    using the Mommertz correlation method [1]_:
+    Calculate the direction dependent free-field scattering coefficient.
+    Uses the Mommertz correlation method [1]_ to calculate the scattering
+    coefficient separately for each channel of the input data:

		with the ``sample_pressure``, the ``reference_pressure``, and the
		area weights ``weights_microphones``. See

		import pyfar as pf


		def paris_formula(coefficients, incident_directions):

		and :math:`\vartheta` and :math:`\varphi` are the incidence
		angle and azimuth angles. Note that the incident directions should be

		@@ -0,0 +1,7 @@
		imkar.scattering
		================

New scattering freefield implementation #12

Are you sure you want to change the base?

New scattering freefield implementation #12

Conversation

ahms5 commented Feb 24, 2023

Changes proposed in this pull request:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahms5 commented Apr 16, 2023

pingelit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahms5 commented May 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-brinkmann left a comment

Choose a reason for hiding this comment

f-brinkmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment