Add SphericalHarmonicsAudio classes #166

tluebeck · 2025-07-06T13:35:55Z

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

Closes #165

Changes proposed in this pull request:

introduce SphericalHarmonicsAudio base class which holds all SH related properties
introduce SphericalHarmonicsTimeData and FrequencyData

f-brinkmann

Thanks for the effort. It generally looks good to me! I suggested one structural change and adding the docs but otherwise its fine to me. I think the testing should be fine. complete functionality is tested for SH signal, initialization tested for each class separately.

f-brinkmann · 2025-07-10T11:26:01Z

spharpy/classes/audio.py

+        # check dimensions
+        if len(data.shape) < 3:
+            raise ValueError("Invalid number of dimensions. Data should have "
+                             "at least 3 dimensions.")
+
+        # set n_max
+        n_max = np.sqrt(data.shape[-2])-1
+        if n_max - int(n_max) != 0:
+            raise ValueError("Invalid number of SH channels: "
+                             f"{data.shape[-2]}. It must match (n_max + 1)^2.")
+        self._n_max = int(n_max)


It seems a little redundant to me to have this in all three classes. Would the following work:

First call the init of the Audio class (FrequencyData in this case)

Second call the init of the SHAudio class and move this code there

Good point, yes that works.

f-brinkmann · 2025-07-10T11:26:25Z

spharpy/classes/audio.py

+
+
+class SphericalHarmonicTimeData(SphericalHarmonicsAudio, TimeData):
+    """_summary_


Missing docs - probably intended to get a review before the effort.

f-brinkmann · 2025-07-10T11:26:34Z

spharpy/classes/audio.py

+
+
+class SphericalHarmonicFrequencyData(SphericalHarmonicsAudio, FrequencyData):
+    """_summary_


Missing docs - probably intended to get a review before the effort.

add docstrings move n_max and dimensions check to base class

f-brinkmann

Looks good to me. I would suggest to wait with the merge until we finalized discussions on the SH conventions that we want to support. This might cause changes here as well.

f-brinkmann · 2025-07-24T13:57:00Z

spharpy/classes/audio.py

+    comment : str
+        A comment related to `data`. The default is ``None``.
+    """
+    def __init__(self, data, frequencies, basis_type, normalization,


Do we want to have default values for the SH parameters as in the SH class, or are there not defaults by intention to make sure people think about this when inputting data from sources other than our internal SHT functions?

ahms5

Thank you for taking care!
just minor comments

ahms5 · 2025-07-31T22:38:28Z

spharpy/classes/audio.py

-class SphericalHarmonicSignal(Signal):
-    """Create audio object with spherical harmonics coefficients in time or
-    frequency domain.
+class SphericalHarmonicAudio(_Audio):


Suggested change

class SphericalHarmonicAudio(_Audio):

class _SphericalHarmonicAudio(_Audio):

shouldnt this be private, too?

ahms5 · 2025-07-31T22:42:44Z

spharpy/classes/audio.py

+                        comment=comment, is_complex=is_complex)
+        SphericalHarmonicAudio.__init__(
+            self, basis_type, normalization, channel_convention,
+            condon_shortley, domain="time", comment=comment)


Suggested change

condon_shortley, domain="time", comment=comment)

condon_shortley, domain=domain, comment=comment)

domain="time"would overwrite the given domain. But i think we can remove it here anyway

ahms5 · 2025-07-31T22:48:51Z

spharpy/classes/audio.py

+    def __init__(self, basis_type, normalization, channel_convention,
+                 condon_shortley, domain, comment=""):
+        _Audio.__init__(self, domain=domain, comment=comment)


Suggested change

def __init__(self, basis_type, normalization, channel_convention,

condon_shortley, domain, comment=""):

_Audio.__init__(self, domain=domain, comment=comment)

def __init__(self, basis_type, normalization, channel_convention,

condon_shortley):

i wounder if we really need it here, because it would be set twice, by the audio class and then overwritten by this class. I think the _Audio.init is not setting anything else then this to entries, so i guess we can remove it.

ahms5 · 2025-07-31T22:49:24Z

spharpy/classes/audio.py

+        SphericalHarmonicAudio.__init__(
+            self, basis_type, normalization, channel_convention,
+            condon_shortley, domain="time", comment=comment)


Suggested change

SphericalHarmonicAudio.__init__(

self, basis_type, normalization, channel_convention,

condon_shortley, domain="time", comment=comment)

SphericalHarmonicAudio.__init__(

self, basis_type, normalization, channel_convention,

condon_shortley)

sikersten · 2025-08-21T12:32:03Z

Thanks for implementing! I think I am missing something fundamental here. Why is it not possible that _SphericalHarmonicAudio inherits from the SphericalHarmonics class?

mberz · 2025-08-22T08:48:15Z

Thanks for implementing! I think I am missing something fundamental here. Why is it not possible that _SphericalHarmonicAudio inherits from the SphericalHarmonics class?

Because the SphericalHarmonics class contains a number of properties which are not meaningful in the SphericalHarmonicSignal class. They however share a set of properties/methods which would make a shared base class usefull, see #173

f-brinkmann

The import of SH TimeData and FrequencyData is missing in spharpy/__init__.py

f-brinkmann · 2025-10-20T14:31:34Z

spharpy/classes/audio.py

+    well as the normalizations N3D, SN3D, or MaxN, see [#]_. The definition of
+    the spherical harmonics basis functions is based on the scipy convention
+    which includes the Condon-Shortley phase, [#]_, [#]_.


I would suggest to keep this plain and without references. We will have a large general documentation for SHs for this:

Suggested change

well as the normalizations N3D, SN3D, or MaxN, see [#]_. The definition of

the spherical harmonics basis functions is based on the scipy convention

which includes the Condon-Shortley phase, [#]_, [#]_.

well as the normalizations N3D, NM, SN3D, SNM, or MaxN.

This re-appears in the SphericalHarmonicSignal class. But I can't comment there because the lines were not changed in this pull.

f-brinkmann · 2025-10-20T14:32:14Z

spharpy/classes/audio.py

+    .. [#] F. Zotter, M. Frank, "Ambisonics A Practical 3D Audio Theory
+            for Recording, Studio Production, Sound Reinforcement, and
+            Virtual Reality", (2019), Springer-Verlag
+    .. [#] B. Rafely, "Fundamentals of Spherical Array Processing", (2015),
+            Springer-Verlag
+    .. [#] E.G. Williams, "Fourier Acoustics", (1999), Academic Press


See above. But maybe wait if anyone disagrees.

Suggested change

.. [#] F. Zotter, M. Frank, "Ambisonics A Practical 3D Audio Theory

for Recording, Studio Production, Sound Reinforcement, and

Virtual Reality", (2019), Springer-Verlag

.. [#] B. Rafely, "Fundamentals of Spherical Array Processing", (2015),

Springer-Verlag

.. [#] E.G. Williams, "Fourier Acoustics", (1999), Academic Press

f-brinkmann

Can you please merge develop into this again to make sure we have capitalized channel conventions 'ACN' and 'FuMa'?

f-brinkmann

Thanks for the effort. The inheritance generally looks good, but I found two things that I think must be considered:

I think we must overwrite the setters for n_max, basis_type, and condon_shortley to raise an error when the user attempts to set these attributes. Otherwise these parameters can be changed but the change won't be refelcted in the data. We could allow for such things, but I would not consider it in this pull request:

td = SphericalHarmonicTimeData([[[1, 2, 3]]], [1, 2, 3], 'real', 'SN3D', 'acn', 'auto')
td.n_max = 1  # this changes the order, but the data stays the same.

We need to handle setting the data and getting n_max. The following changes the data, without changing n_max. I assume we want to be able to set new data and updating the order accordingly, e.g., whan upmixing to a higher SH order:

td = sy.classes.SphericalHarmonicTimeData([[[1, 2, 3]]], [1, 2, 3], 'real', 'SN3D', 'acn', 'auto')
td.time = np.random.rand(1, 4, 100)

f-brinkmann · 2025-10-20T14:57:09Z

spharpy/classes/audio.py

+
+    """
+    def __init__(self, basis_type, normalization, channel_convention,
+                 condon_shortley, domain, comment=""):


domain and time are not accessed here, I assume because they are set in the audio classes from this inherits.

Suggested change

condon_shortley, domain, comment=""):

condon_shortley):

f-brinkmann · 2025-10-20T14:59:15Z

spharpy/classes/audio.py

+    harmonic coefficients and times.
+
+    Objects of this class contain time data which is not directly convertible
+    to frequency domain, i.e., non-equidistant samples.


Suggested change

to frequency domain, i.e., non-equidistant samples.

to the frequency domain, i.e., non-equidistant samples.

f-brinkmann · 2025-10-20T15:01:49Z

spharpy/classes/audio.py

+        ``'maxN'``, ``'SN3D'`` or ``'SNM'``. (maxN is only supported up
+        to 3rd order)
+    channel_convention : str
+        Channel ordering convention, either ``'acn'`` or ``'fuma'``.


Suggested change

Channel ordering convention, either ``'acn'`` or ``'fuma'``.

Channel ordering convention, either ``'ACN'`` or ``'FuMa'``.

f-brinkmann · 2025-10-20T15:02:23Z

spharpy/classes/audio.py

+        Flag to indicate if the Condon-Shortley phase term is included
+        (``True``) or not (``False``).
+    comment : str
+        A comment related to `data`. The default is ``None``.


Suggested change

A comment related to `data`. The default is ``None``.

A comment related to `data`. The default is ``""``.

f-brinkmann · 2025-10-20T15:03:51Z

spharpy/classes/audio.py

+        coefficients with 1024 samples each. The data can be ``int`` or
+        ``float`` and is converted to ``float`` in any case.


To be consistent with FrequencyData

Suggested change

coefficients with 1024 samples each. The data can be ``int`` or

``float`` and is converted to ``float`` in any case.

coefficients with 1024 samples each. The data can be ``int``,

``float`` or ``complex``. Data of type ``int`` is converted to

``float``.

f-brinkmann · 2025-10-20T15:08:51Z

spharpy/classes/audio.py

+        ``'maxN'``, ``'SN3D'`` or ``'SNM'``. (maxN is only supported up
+        to 3rd order)
    channel_convention : str
        Channel ordering convention, either ``'acn'`` or ``'fuma'``.


Suggested change

Channel ordering convention, either ``'acn'`` or ``'fuma'``.

Channel ordering convention, either ``'ACN'`` or ``'FuMa'``.

f-brinkmann · 2025-10-20T15:10:23Z

spharpy/classes/audio.py

-            self._channel_convention = value
+        _SphericalHarmonicAudio.__init__(
+            self, basis_type, normalization, channel_convention,
+            condon_shortley, domain=domain, comment=comment)


see comment above:

Suggested change

condon_shortley, domain=domain, comment=comment)

condon_shortley)

f-brinkmann · 2025-10-20T15:18:22Z

tests/test_spherical_harmonics_audio.py

+        data, times,  basis_type='real', normalization='SN3D',
+        channel_convention="acn", condon_shortley=False,
+        comment="")
+    assert isinstance(sh_time_data, SphericalHarmonicTimeData)


This would be more strict

Suggested change

assert isinstance(sh_time_data, SphericalHarmonicTimeData)

assert type(sh_time_data) == SphericalHarmonicTimeData

(maybe needs is instead of ==)

f-brinkmann · 2025-10-20T15:18:38Z

tests/test_spherical_harmonics_audio.py

+        data, frequencies, basis_type='real', normalization='SN3D',
+        channel_convention="acn", condon_shortley=False,
+        comment="")
+    assert isinstance(sh_freq_data, SphericalHarmonicFrequencyData)


Suggested change

assert isinstance(sh_freq_data, SphericalHarmonicFrequencyData)

assert type(sh_freq_data) == SphericalHarmonicFrequencyData

f-brinkmann · 2025-10-20T15:20:07Z

tests/test_spherical_harmonics_signal.py

+        match="Invalid channel convention, currently only 'acn' "
+              "and 'fuma' are supported"):


Suggested change

match="Invalid channel convention, currently only 'acn' "

"and 'fuma' are supported"):

match="Invalid channel convention, currently only 'ACN' "

"and 'FuMa' are supported"):

Should also be changed below after merging develop into this.

f-brinkmann

Sorry, forgot one thing. I would suggest to rename audio.py to sh_audio.py.

mberz · 2025-10-22T06:11:03Z

Sorry, forgot one thing. I would suggest to rename audio.py to sh_audio.py.

Please rename files in this pr. this will cause conflicts with other branches.

mberz · 2025-10-23T18:52:50Z

Sorry, forgot one thing. I would suggest to rename audio.py to sh_audio.py.

Please rename files in this pr. this will cause conflicts with other branches.

I'm very sorry, I meant to write don't rename in this branch, since the file is already part of develop.
Could you please revert dfd4d54 ?

mberz

I'm just commenting on this now, as I feel we need to make some adjustments to the SphericalHarmonicDefinition class before continuing with this PR.

I think it makes sense to move the functionality of saving the old properties to the parent class, as it could come in handy in other classes.
I now remember why I first had the SphericalHarmonicDefintion in mind without the n_max property defined. I'll add an abstract base class for that which could serve as parent for the SphericalHarmonicAudio classes.

Sorry to postpone this PR once again.

mberz · 2025-08-08T09:16:30Z

spharpy/classes/audio.py

+    three sub-classes :py:func:`SphericalHarmonicsTimeData`,
+    :py:func:`SphericalHarmonicsFrequencyData`, and
+    :py:func:`SphericalHarmonicsSignal`.


Suggested change

three sub-classes :py:func:`SphericalHarmonicsTimeData`,

:py:func:`SphericalHarmonicsFrequencyData`, and

:py:func:`SphericalHarmonicsSignal`.

three sub-classes :py:func:`SphericalHarmonicTimeData`,

:py:func:`SphericalHarmonicFrequencyData`, and

:py:func:`SphericalHarmonicSignal`.

Sure, reverted. I kind of had the feeling it was a typo cause it did't made sense to me :D ...
I agree that it makes sense to move the saving of the old parameters to the SHDefinitions class. However, I think it could be an option to do that in a separate PR. The functionality from outside would not change, and we could move on with other stuff, like SHtrafo etc.

add SHAudio SHTimeData and SHFrequencyData

6c64125

tluebeck changed the base branch from main to develop July 6, 2025 13:36

tluebeck self-assigned this Jul 6, 2025

tluebeck added the enhancement New feature or request label Jul 6, 2025

tluebeck marked this pull request as draft July 6, 2025 13:37

tluebeck added 2 commits July 6, 2025 15:54

add tests

700bcb9

add doc and clean up

67b163f

tluebeck requested review from ahms5, f-brinkmann, mberz and sikersten July 7, 2025 06:55

fix typo

d4e0556

mberz added this to Weekly Planning Jul 8, 2025

github-project-automation bot moved this to Backlog in Weekly Planning Jul 8, 2025

mberz moved this from Backlog to Drafting Phase in Weekly Planning Jul 8, 2025

f-brinkmann requested changes Jul 10, 2025

View reviewed changes

github-project-automation bot moved this from Drafting Phase to Require review in Weekly Planning Jul 10, 2025

tluebeck added 2 commits July 11, 2025 07:24

fix typo

90d05ba

update according to review

40a973b

add docstrings move n_max and dimensions check to base class

tluebeck requested a review from f-brinkmann July 18, 2025 11:14

f-brinkmann approved these changes Jul 24, 2025

View reviewed changes

github-project-automation bot moved this from Require review to Reviewer Approved in Weekly Planning Jul 24, 2025

f-brinkmann moved this from Reviewer Approved to Require review in Weekly Planning Jul 24, 2025

ahms5 added this to the v1.0.0 milestone Jul 25, 2025

ahms5 requested changes Jul 31, 2025

View reviewed changes

update according to review

de6eaaa

ahms5 modified the milestones: v1.0.0, v1.1.0 Sep 12, 2025

f-brinkmann moved this from Require review to Implementation in progress in Weekly Planning Sep 27, 2025

tluebeck added 5 commits October 3, 2025 19:18

Merge branch 'develop' into dev/spherical_harmonics_audio

490dfa8

adapt to new normalization names

3424e41

Merge branch 'develop' into dev/spherical_harmonics_audio

40785fa

add SphericalharmonicDefinition to SHAudio Classes

ef86b74

ruff checks

d991c26

tluebeck marked this pull request as ready for review October 19, 2025 01:43

f-brinkmann self-requested a review October 20, 2025 11:56

f-brinkmann reviewed Oct 20, 2025

View reviewed changes

f-brinkmann requested changes Oct 20, 2025

View reviewed changes

github-project-automation bot moved this from Implementation in progress to Require review in Weekly Planning Oct 20, 2025

f-brinkmann reviewed Oct 20, 2025

View reviewed changes

tluebeck added 2 commits October 21, 2025 19:28

update according to review

a1bf902

Merge branch 'develop' into dev/spherical_harmonics_audio

b3be88d

tluebeck added 2 commits October 22, 2025 18:38

update according ot review

a49f4cd

rename audio to sh_audio

dfd4d54

mberz reviewed Oct 23, 2025

View reviewed changes

revert renaming of sh_audio

6666f3a

mberz mentioned this pull request Oct 24, 2025

Abstract base class for SphericalHarmonicDefiniton and -Audio #218

Draft



		class SphericalHarmonicTimeData(SphericalHarmonicsAudio, TimeData):
		"""_summary_



		class SphericalHarmonicFrequencyData(SphericalHarmonicsAudio, FrequencyData):
		"""_summary_

	class SphericalHarmonicAudio(_Audio):
	class _SphericalHarmonicAudio(_Audio):

	condon_shortley, domain="time", comment=comment)
	condon_shortley, domain=domain, comment=comment)

	to frequency domain, i.e., non-equidistant samples.
	to the frequency domain, i.e., non-equidistant samples.

	Channel ordering convention, either ``'acn'`` or ``'fuma'``.
	Channel ordering convention, either ``'ACN'`` or ``'FuMa'``.

	A comment related to `data`. The default is ``None``.
	A comment related to `data`. The default is ``""``.

		coefficients with 1024 samples each. The data can be ``int`` or
		``float`` and is converted to ``float`` in any case.

-        coefficients with 1024 samples each. The data can be ``int`` or
-        ``float`` and is converted to ``float`` in any case.
+        coefficients with 1024 samples each. The data can be ``int``,
+        ``float`` or ``complex``. Data of type ``int`` is converted to
+        ``float``.

	condon_shortley, domain=domain, comment=comment)
	condon_shortley)

	assert isinstance(sh_time_data, SphericalHarmonicTimeData)
	assert type(sh_time_data) == SphericalHarmonicTimeData

	assert isinstance(sh_freq_data, SphericalHarmonicFrequencyData)
	assert type(sh_freq_data) == SphericalHarmonicFrequencyData

		match="Invalid channel convention, currently only 'acn' "
		"and 'fuma' are supported"):

Uh oh!

Add SphericalHarmonicsAudio classes #166

Are you sure you want to change the base?

Add SphericalHarmonicsAudio classes #166

Conversation

tluebeck commented Jul 6, 2025

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

Changes proposed in this pull request:

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!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahms5 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!

sikersten commented Aug 21, 2025

Uh oh!

mberz commented Aug 22, 2025

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!

f-brinkmann left a comment

Choose a reason for hiding this comment

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!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

mberz commented Oct 22, 2025

Uh oh!