Feature/add sh axis property multichannel by tluebeck · Pull Request #329 · pyfar/spharpy

tluebeck · 2026-05-18T14:01:29Z

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

Closes #326

Changes proposed in this pull request:

Introduce property caxis_spherical_harmonics in _SphericalHarmonicsAudio, SphericalHarmonicTimeData , SphericalHarmonicFrequencyData, and SphericalHarmonicSignal which defines the axis along which SHSignals store SH coefficients
Adapt rotation accordingly
allow caxis_spherical_harmonics property for multichannel SH data

implement tests

Adapt docstrings

change property name adapt tests

Allow caxis_spherical_harmonics to be a tuple

…signals

mberz

Looks very much feasible!
I think we'll need an update to the n_max property and additional checks to ensure that the order of each dimensions corresponding to spherical harmonic data matches.
Or would you suggest to allow different orders for each dimension?

mberz · 2026-05-21T16:40:07Z

+    # check if all sh_channels match
+    sh_channels = [data.shape[a] for a in axis]
+
+    if len(set(sh_channels)) != 1:
+        raise ValueError(
+            "All SH axes must have the same number of channels, "
+            f"but got {sh_channels}"
+        )


I think this check should not be necessary, since the order governs the total number of channels. So all axes containing spherical harmonic data should have the same length.

So this check should probably be moved to the respective SphericalHarmonic* classes.

mberz · 2026-05-21T16:40:53Z

+    # check if all sh_channels match
+    sh_channels = [data.shape[a] for a in axis]
+
+    if len(set(sh_channels)) != 1:


same comment as above. Implementing the check also makes individual checks in each function obsolete.

move check if all spherical harmonics axes have equal length to data init

test change_channel_convention and renormalize with mutlichannel data

implement tests

Adapt docstrings

change property name adapt tests

…property_multichannel

f-brinkmann

Thanks for pushing ths forward. Docs look mainly good but I think I found one bug and have three things for general discussion. I did not yet review the tests.

I think renormalize does not work as intended yet (see detailed comment). If true, tests should be adapted to detect the unwanted behavior and test the desired.
I think it would be more consistent and easier to handle if caxis_spherical_harmonics would always be a tuple.
This is the biggest: We are inheriting from pyfar._Audio which has the reshape, transpose and T methods that move around axes. I think we need to overload them in _SphericalHarmonicAudio to change caxis_spherical_harmonics accordingly. Should be rather straigt forward: call the parent function first, then update caxis_spherical_harmonics. For saftey I would suggest to check dimensions again and would suggest to add a private setter for caxis_spherical_harmonics to _SphericalHarmonicAudio that does the checks. This would also reduce code redundancy, because these checks are implemented three times at the moment (once in each SH Audio class).

f-brinkmann · 2026-09-15T14:26:04Z

+        samples and the second to last the spherical harmonic coefficients. If
+        the raw data have more then 3 dimensions `caxis_spherical_harmonics`
+        defines the axis holding the spherical harmonic coefficients. The
+        default is -1 (second to last channel axis). Accordingly, the default


should be last caxis or second to last axis

Suggested change

default is -1 (second to last channel axis). Accordingly, the default

default is -1 (last channel axis). Accordingly, the default

(should be changed everywhere)

f-brinkmann · 2026-09-15T14:27:07Z

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


Suggested change

`caxis_spherical_harmonics` = -1. The data can be ``int``, ``float``

a `caxis_spherical_harmonics` of -1. The data can be ``int``, ``float``

or

Suggested change

`caxis_spherical_harmonics` = -1. The data can be ``int``, ``float``

``caxis_spherical_harmonics = -1``. The data can be ``int``, ``float``

(should be changed everywhere)

f-brinkmann · 2026-09-15T14:31:38Z

+        if abs(caxis_spherical_harmonics) > data.ndim:
+            raise ValueError(
+                    f"caxis_spherical_harmonics ({caxis_spherical_harmonics}) "
+                    f"exceeds the number of dimensions of data ({data.ndim})")


I think this fails if caxis_spherical_harmonics is a tuple. It can probably be omitted because it is checked below as well.

f-brinkmann · 2026-09-15T14:32:20Z

+        if isinstance(caxis_spherical_harmonics, int):
+            caxis_spherical_harmonics = (caxis_spherical_harmonics, )
+
+        for caxis in caxis_spherical_harmonics:
+            if abs(caxis) > data.ndim:
+                raise ValueError(
+                    f"caxis_spherical_harmonics "
+                    f"({caxis}) exceeds the number "
+                    f"of dimensions of data ({data.ndim})")


Isn't this code identical for all three SHAudio classes? I would suggest to put it into a private function to make it reusable

f-brinkmann · 2026-09-15T14:33:30Z

+            caxis_spherical_harmonics = (caxis_spherical_harmonics, )
+
+        for caxis in caxis_spherical_harmonics:
+            if abs(caxis) > data.ndim:


Wouldn't this fail, if data is an list but not an ndarray? I think this should be moved after data = _atleast_3d_first_dimension(data)

f-brinkmann · 2026-09-15T15:00:53Z

+        sh_caxis = target.caxis_spherical_harmonics - 1
+
+        # move SH axis to front
+        data = np.moveaxis(data, sh_caxis, 0)


The numpy docs say that source and destination axes must be unique, so I assume this must be

Suggested change

data = np.moveaxis(data, sh_caxis, 0)

data = np.moveaxis(data, sh_caxis, np.arange(len(sh_caxis)))

for things to work if we a have a tuple of SH caxes

f-brinkmann · 2026-09-15T15:01:06Z

+        rotated_data = np.tensordot(M, data, axes=(1, 0))
+
+        # move SH axis back to original position
+        rotated_data = np.moveaxis(rotated_data, 0, sh_caxis)


f-brinkmann · 2026-09-16T06:10:09Z

+    if isinstance(axis, int):
+        axis = (axis,)


Would not be required anymore if axis was always a tuple.

I changed it for all the private methods, but I think for convenience, it would be nice to allow both, int and tuple here for the public methods.

f-brinkmann · 2026-09-16T06:20:12Z

    shape = [1] * data.ndim
-    shape[axis] = data.shape[axis]
+
+    shape[axis[0]] = sh_channels


I think this does not work as intended yet. I get the same result for renormalizing an array with a single and two SH axes:

import spharpy import numpy as np data = np.ones((1, 4, 4, 8)) channel_convention = 'ACN' current_norm = 'N3D' target_norm = 'SN3D' data1 = spharpy.spherical.renormalize( data, channel_convention, current_norm, target_norm, (1, )) data2 = spharpy.spherical.renormalize( data, channel_convention, current_norm, target_norm, (1, 2)) np.all(data1 == data2) # is True but should not be true (?)

f-brinkmann · 2026-09-16T06:23:38Z

+    if isinstance(axis, int):
+        axis = (axis,)


Would not be required anymore of axis was always a tuple.

- bugfix normalization - edit caxis_spherical_harmonic getter - refactor _assert_valid_number_of_sh_channels and _convert_to_standard_definition - update tests

- override transpose

- implement override reshape - bugfix rotation

tluebeck · 2026-09-17T13:12:23Z

Thanks for the review. Good catches. I think everything is adapted and fixed now. Maybe we should discuss on the new behavior of reshaping SHsignals. Do we want to reshape caxis_spherical_harmonics as well? I think that wouldn't make sense.

f-brinkmann · 2026-09-18T11:33:22Z

In addition to the above, I noticed that _Audio.flatten() might also be difficult because it would interleave SH channels with non SH channels. Running an LLM on all our repos found the following uses on a Signal level:

all pyfar.plot functions flatten the signal
pyfar/dsp/filter/gammatone.py:391 and pyfar/dsp/interpolation.py:801 and some pyrato.edc methods flatten the input and reshape after processing

All usages are fine since _Audio.flatten() returns a copy of the Signal. Because all functions could be usefull with an SH Signal, how can we make this work?

SHAudio.flatten could return an Audio object (not SHAudio)
gammatone and interpolation would return an SHAudio object (fine)
EDC methods return TimeData and information about the SH axes will be lost (not ideal)

f-brinkmann

Thanks for the quick implementation. Reshaping seems to get really annoying. Maybe something we can discuss in general.

f-brinkmann · 2026-09-18T11:34:54Z


-def _assert_valid_number_of_sh_channels(shape, sh_axis):
+def _assert_valid_caxis_spherical_harmonics(data, caxis_spherical_harmonics):
+    """Check if the spherical harmonic channel axes are ``ìnt`` and do


Typo should be 'i' not 'ì'

f-brinkmann · 2026-09-18T11:37:14Z

+    ------
+    ValueError
+        Raised if the number of spherical harmonic channels does not match
+        (n_max + 1)^2 for an integer n_max.


This is checked below. Can you update this please?

f-brinkmann · 2026-09-18T11:39:17Z

+
+    Parameters
+    ----------
+    data : numpy.ndarray


I would suggest to directly pass data.ndim

f-brinkmann · 2026-09-18T11:46:33Z

+        Return reshaped copy of the SphericalHarmonicAudio object. Axes
+        containing the spherical harmonics coefficients can not be reshaped.


Did not think of this before to be honest. A different solution would be to allow freely reshaping and return an Audio either always or if SH channels are affected by reshaping.

f-brinkmann · 2026-09-18T11:49:57Z

+        new_caxis_sh = []
+        for caxis in self._caxis_spherical_harmonics:
+            n_sh_channels = old_cshape[caxis]
+            if n_sh_channels not in new_cshape:
+                raise ValueError(
+                    "The requested new shape does not leave a "
+                    "channel axis to hold the spherical "
+                    "harmonic channels.")


Not sure if this always works. Could fail in case the size of a non-SH channel matches that of an SH channel.

f-brinkmann · 2026-09-18T11:58:25Z

-    shape = [1] * data.ndim
-
-    shape[axis[0]] = sh_channels
+    # calculate one renormalization factor per sh channel


Is this required? All SH channels have the same order and hence the same normalization will be applied everywhere.

f-brinkmann · 2026-09-18T11:59:16Z

+        shape = [1] * data.ndim
+        shape[a] = sh_channels
+
+        data_renorm *= factor.reshape(shape)


see comment above - the same factor is applied all the time, which is correct.

tluebeck added 13 commits April 25, 2026 08:05

Introduce property sh_caxis

a0a1bb8

adapt convert_to/from_standard_definition

a36ab7a

implement tests

apply rotation along sh_caxis

a4d8776

add docstrings

fbce6bc

add check for positive sh_caxis

4acabce

Add tests for checking positive sh_caxes

20aad5d

changed default sh_caxis

687c6c5

add test for valid sh_caxis

03ee888

add tests for sh_caxis checks

2695077

update according to review.

e8d3ec0

Adapt docstrings

update according to review

d345de4

change property name adapt tests

Implement multichannel SHSignals

e6fd0b2

Allow caxis_spherical_harmonics to be a tuple

convert caxis_spherical_harmonics to tuple

bdefd02

tluebeck requested a review from mberz May 18, 2026 14:01

change_channel_convention and renormalize can handle multichannel sh_…

87caec2

…signals

mberz changed the base branch from develop to feature/add_sh_axis_property May 21, 2026 16:33

mberz reviewed May 21, 2026

View reviewed changes

tluebeck added 3 commits May 22, 2026 10:31

refactor check for sh_axis

0a3e832

move check if all spherical harmonics axes have equal length to data init

implement multichannel tests

b423d25

test change_channel_convention and renormalize with mutlichannel data

edit caxis_spherical_harmonics getter, fix tests

e51b6dc

tluebeck added the feature label Sep 9, 2026

tluebeck added this to the v1.1.0 milestone Sep 9, 2026

tluebeck marked this pull request as ready for review September 9, 2026 14:35

tluebeck requested a review from f-brinkmann September 9, 2026 14:47

bugfix sh_axis in rotation

e3b5f79

tluebeck requested a review from mberz September 9, 2026 15:28

tluebeck added 2 commits September 10, 2026 08:35

fix ruff tests

cdb68b6

Merge branch 'develop' into feature/add_sh_axis_property_multichannel

648c8ed

f-brinkmann requested review from a team and ahms5 September 14, 2026 08:49

f-brinkmann requested review from hoyer-a and sikersten September 14, 2026 08:49

tluebeck added 11 commits September 14, 2026 11:19

Introduce property sh_caxis

8111de7

adapt convert_to/from_standard_definition

aeef7b3

implement tests

apply rotation along sh_caxis

e1c0f6f

add docstrings

a27e111

add check for positive sh_caxis

bf9245a

Add tests for checking positive sh_caxes

cd3eb40

changed default sh_caxis

0178868

add test for valid sh_caxis

0bc89ea

add tests for sh_caxis checks

4145e41

update according to review.

3f9cc1e

Adapt docstrings

update according to review

a6f6306

change property name adapt tests

f-brinkmann force-pushed the feature/add_sh_axis_property branch from d345de4 to a6f6306 Compare September 14, 2026 09:20

Merge branch 'feature/add_sh_axis_property' into feature/add_sh_axis_…

d470f0c

…property_multichannel

tluebeck changed the base branch from feature/add_sh_axis_property to develop September 15, 2026 07:47

tluebeck mentioned this pull request Sep 15, 2026

Introduce property sh_caxis #327

Closed

f-brinkmann requested changes Sep 16, 2026

View reviewed changes

tluebeck added 3 commits September 17, 2026 09:54

update according to review

4e97c62

- bugfix normalization - edit caxis_spherical_harmonic getter - refactor _assert_valid_number_of_sh_channels and _convert_to_standard_definition - update tests

update according to review 2

bfb902f

- override transpose

update according to review 3

4140ef6

- implement override reshape - bugfix rotation

tluebeck requested a review from f-brinkmann September 17, 2026 13:12

fix ruff and override flatten

b75e937

f-brinkmann requested changes Sep 18, 2026

View reviewed changes

	default is -1 (second to last channel axis). Accordingly, the default
	default is -1 (last channel axis). Accordingly, the default

	`caxis_spherical_harmonics` = -1. The data can be ``int``, ``float``
	a `caxis_spherical_harmonics` of -1. The data can be ``int``, ``float``

	`caxis_spherical_harmonics` = -1. The data can be ``int``, ``float``
	``caxis_spherical_harmonics = -1``. The data can be ``int``, ``float``

	data = np.moveaxis(data, sh_caxis, 0)
	data = np.moveaxis(data, sh_caxis, np.arange(len(sh_caxis)))

		Return reshaped copy of the SphericalHarmonicAudio object. Axes
		containing the spherical harmonics coefficients can not be reshaped.

Conversation

tluebeck commented May 18, 2026 • 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!

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

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!

tluebeck commented Sep 17, 2026

Uh oh!

f-brinkmann commented Sep 18, 2026

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tluebeck commented May 18, 2026 •

edited

Loading