[ENH] Classmethod to create SphericalHarmonicSignal from SphericalHarmonicDefinition #247

tluebeck · 2025-12-15T13:36:19Z

Changes proposed in this pull request:

add SphericalHarmonics constructor which creates SH signal from SHdefinition object

Note: Tests are failing because of #259

add docstrings move n_max and dimensions check to base class

Always store fully normalized data in ACN order and renormalize accordinglz in setter/getter

manually integrate branch #229

allow to pass sh data with dimensions < 3

overwrite basis_type setter and freq_raw

init sh signal from definition

mberz

Thanks for the draft. I did a quick conceptual review.
Main point is to harmonize it with #249

mberz · 2025-12-19T11:32:49Z

spharpy/classes/audio.py

+    def from_spherical_harmonics_definition(
+            cls, data, times, sh_definition, domain='time',
+            fft_norm='none', comment="", is_complex=False):


Suggested change

def from_spherical_harmonics_definition(

cls, data, times, sh_definition, domain='time',

fft_norm='none', comment="", is_complex=False):

def from_definition(

cls, sh_definition, data, times, domain='time',

fft_norm='none', comment="", is_complex=False):

I'd suggest to change the order and shorten the name as in #249

mberz · 2025-12-19T11:32:53Z

spharpy/classes/audio.py

+        Create a SphericalHarmonicsSignal class object from
+        SphericalHarmonicsDefinition object, data, and sampling
+        rate.


Just noting this so it's not forgotten:

i) Please use the correct names, also see occurences below.

Suggested change

Create a SphericalHarmonicsSignal class object from

SphericalHarmonicsDefinition object, data, and sampling

rate.

Create a SphericalHarmonicSignal class object from

SphericalHarmonicDefinition object, data, and sampling

rate.

ii) Classmethods should get parameter documentation

mberz · 2025-12-19T11:36:58Z

spharpy/classes/audio.py

+    @property
+    def freq(self):
+        """Return or set the data in the frequency domain."""
+        return _convert_from_standard_definition(Signal.freq.fget(self),
+                                                 self.normalization,
+                                                 self.channel_convention)
+
+    @freq.setter
+    def freq(self, value):
+        """Return or set the data in the frequency domain."""


Suggested change

@property

def freq(self):

"""Return or set the data in the frequency domain."""

return _convert_from_standard_definition(Signal.freq.fget(self),

self.normalization,

self.channel_convention)

@freq.setter

def freq(self, value):

"""Return or set the data in the frequency domain."""

I have the feeling that this is here by mistake?

mberz · 2025-12-19T11:38:03Z

tests/test_spherical_harmonics_signal.py

+                       match="Invalid basis type, only "
+                             "'complex' and 'real' are supported"):


unrelated, please keep the diff clean.

change names change order of parameters add parameter documentation

f-brinkmann

Thanks! I only found a repeating typo and made a suggestion for shortening a line in the tests.

f-brinkmann · 2025-12-24T08:32:19Z

spharpy/classes/audio.py

+            cls, sh_definition, data, times, comment="", is_complex=False):
+        r"""
+        Create a SphericalHarmonicTimeData class object from
+        SphericalHarmonicsDefinition object, data, and times.


Suggested change

SphericalHarmonicsDefinition object, data, and times.

SphericalHarmonicDefinition object, data, and times.

f-brinkmann · 2025-12-24T08:33:56Z

spharpy/classes/audio.py

+            cls, sh_definition, data, frequencies, comment=""):
+        r"""
+        Create a SphericalHarmonicFrequencyData class object from
+        SphericalHarmonicsDefinition object, data, and frequencies


Suggested change

SphericalHarmonicsDefinition object, data, and frequencies

SphericalHarmonicDefinition object, data, and frequencies

f-brinkmann · 2025-12-24T08:35:29Z

spharpy/classes/audio.py

+            fft_norm='none', comment="", is_complex=False):
+        r"""
+        Create a SphericalHarmonicsSignal class object from
+        SphericalHarmonicsDefinition object, data, and sampling


Suggested change

SphericalHarmonicsDefinition object, data, and sampling

SphericalHarmonicDefinition object, data, and sampling

Thanks for the review. While fixing this typo I realized that I generally used spherical harmonic / harmonicS quite interchangeably. Actually, we (mostly I) used it interchangeably throughout multiple docstrings. I will probably take care in a separate PR.

f-brinkmann · 2025-12-24T08:37:17Z

tests/test_spherical_harmonics_audio.py

+    assert not deepdiff.DeepDiff(
+        time_data_def.__dict__, time_data.__dict__)


Could we use assert time_data == time_data_def here? I think we overloaded the == operator.

f-brinkmann · 2025-12-24T08:37:31Z

tests/test_spherical_harmonics_audio.py

+    assert not deepdiff.DeepDiff(
+        freq_data_def.__dict__, freq_data.__dict__)


f-brinkmann · 2025-12-24T08:37:48Z

tests/test_spherical_harmonics_signal.py

+    assert not deepdiff.DeepDiff(
+        signal_def.__dict__, signal.__dict__)


ahms5

looks good, just a very tiny remark,

ahms5 · 2025-12-29T09:21:40Z

tests/test_spherical_harmonics_signal.py

+                    match="Invalid basis type, only "
+                        "'complex' and 'real' are supported"):


Suggested change

match="Invalid basis type, only "

"'complex' and 'real' are supported"):

match="Invalid basis type, only "

"'complex' and 'real' are supported"):

unrelated change

f-brinkmann

I think the failing tests for python 3.12 and 3.13 are caused by Unidata/netcdf4-python#1461 (noted by Anton on slack).

To fix tests for 3.11 you probably have to merge develop into your branch to get #259.

Otherwise approved.

tluebeck added 30 commits July 6, 2025 15:33

add SHAudio SHTimeData and SHFrequencyData

6c64125

add tests

700bcb9

add doc and clean up

67b163f

fix typo

d4e0556

fix typo

90d05ba

update according to review

40a973b

add docstrings move n_max and dimensions check to base class

update according to review

de6eaaa

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

update according to review

a1bf902

Merge branch 'develop' into dev/spherical_harmonics_audio

b3be88d

update according ot review

a49f4cd

rename audio to sh_audio

dfd4d54

revert renaming of sh_audio

6666f3a

Merge branch 'develop' into dev/spherical_harmonics_audio

2fce301

Incorporate new SHBase class

e9c4fb0

override time and freq setter in SHaudio

cc245e2

Change normalization and channel_order behaiviour

001de12

Always store fully normalized data in ACN order and renormalize accordinglz in setter/getter

udpate according to review

25dcd87

add data init methods

9348b08

manually integrate branch #229

clean up tests

c07b683

update tests

020af4c

allow to pass sh data with dimensions < 3

fix ruff errors

b05cb04

clean up

5d1015b

update according to review

4ae1205

update according to review

8276700

overwrite basis_type setter and freq_raw

update tests

015e14d

tluebeck added 3 commits December 10, 2025 11:08

Merge branch 'develop' into dev/spherical_harmonics_audio

1b55b35

add constructor

8dd6fd0

init sh signal from definition

Merge branch 'develop' into feature/add_sh_audio_constructors

d2bd86d

tluebeck self-assigned this Dec 15, 2025

tluebeck added the feature label Dec 15, 2025

tluebeck added this to the v1.1.0 milestone Dec 15, 2025

ruf fix

7876ba9

tluebeck mentioned this pull request Dec 16, 2025

[ENH] Initialize spherical harmonics from definition #249

Merged

tluebeck marked this pull request as draft December 17, 2025 07:57

tluebeck added 2 commits December 17, 2025 12:27

Merge branch 'develop' into feature/add_sh_audio_constructors

0db8ed7

add new inits for time and frequency data

4b245c1

mberz requested changes Dec 19, 2025

View reviewed changes

mberz changed the title ~~add SphericalHarmonicsSignal constructor~~ [ENH] Classmethod to create SphericalHarmonicsSignal from SphericalHarmonicDefinitoon Dec 19, 2025

Update according to review

94c548a

change names change order of parameters add parameter documentation

tluebeck marked this pull request as ready for review December 22, 2025 11:53

tluebeck changed the title ~~[ENH] Classmethod to create SphericalHarmonicsSignal from SphericalHarmonicDefinitoon~~ [ENH] Classmethod to create SphericalHarmonicSignal from SphericalHarmonicDefinition Dec 22, 2025

Merge branch 'develop' into feature/add_sh_audio_constructors

5007756

tluebeck requested review from ahms5, f-brinkmann and mberz December 22, 2025 12:01

f-brinkmann reviewed Dec 24, 2025

View reviewed changes

update according to review

43a90b0

ahms5 approved these changes Dec 29, 2025

View reviewed changes

update according to review

12e4a19

tluebeck requested a review from f-brinkmann January 5, 2026 12:09

f-brinkmann reviewed Jan 5, 2026

View reviewed changes

Merge branch 'develop' into feature/add_sh_audio_constructors

62ec75f

		match="Invalid basis type, only "
		"'complex' and 'real' are supported"):

	SphericalHarmonicsDefinition object, data, and times.
	SphericalHarmonicDefinition object, data, and times.

		assert not deepdiff.DeepDiff(
		time_data_def.__dict__, time_data.__dict__)

		assert not deepdiff.DeepDiff(
		freq_data_def.__dict__, freq_data.__dict__)

		assert not deepdiff.DeepDiff(
		signal_def.__dict__, signal.__dict__)

[ENH] Classmethod to create SphericalHarmonicSignal from SphericalHarmonicDefinition #247

Are you sure you want to change the base?

[ENH] Classmethod to create SphericalHarmonicSignal from SphericalHarmonicDefinition #247

Uh oh!

Conversation

tluebeck commented Dec 15, 2025 • edited by f-brinkmann Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

ahms5 left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tluebeck commented Dec 15, 2025 •

edited by f-brinkmann

Loading