Implement different normalization and phase conventions for spherical harmonic basis matrices #102

mberz · 2025-02-20T18:38:20Z

Taken from #82

Co-authored-by: Xenofon Karakonstantis <[email protected]>

…ion to tests

…ntion

add reference SH bases as csv

mberz

Thanks for cleaning this up and implementing the tests.
I can only comment since I opened the PR ;)

I have a couple of comments. Some I only noted in single places, but apply to all functions. It's probably easy to spot.

Could you especially please add a test against the old reference files to ensure that the new "default" settings still agree with the old behavior of the functions.

mberz · 2025-03-06T18:03:38Z

spharpy/spherical.py

+    if not isinstance(coordinates, pf.Coordinates):
+        axis = np.where(coordinates.shape == 3)[0][0]
+        if axis == 0:
+            coordinates = coordinates.T
+        coordinates = pf.Coordinates(coordinates[:, 0],
+                                     coordinates[:, 1],
+                                     coordinates[:, 2])


I don't think we should do this transform. We should stick to allowing only pf.Coordinates or Samplings here.

It was a leftover from the old branch which I did not touch. But I removed it now.

mberz · 2025-03-06T18:05:03Z

spharpy/spherical.py

+        if channel_convention == "fuma":
+            order, degree = fuma_to_nm(acn)
+        else:
+            order, degree = acn_to_nm(acn)
        basis[:, acn] = _special.spherical_harmonic(
-            order,
-            degree,
-            coords.colatitude,
-            coords.azimuth)
-
+            order, degree, coordinates.colatitude, coordinates.azimuth
+        )
+        if normalization == "sn3d":
+            basis[:, acn] *= n3d_to_sn3d_norm(order)
+        elif normalization == "maxN":
+            basis[:, acn] *= n3d_to_maxn(acn)
+        if phase_convention is None:
+            # Condon-Shortley phase term is already included in
+            # the special.spherical_harmonic function
+            # so need to divide by (-1)^m
+            basis[:, acn] /= (-1) ** float(degree)


Couldn't these re-normalization and re-sorting operations be moved out of the loop? This should be much faster.

+1 for this. I think scipy.special.sph_har can also generate the entire matrix without being called in the loop, but that would require refactoring our special module. Could be a speed-up for high orders and large coordinates objects

I can see the benefit but the gains are minimal if you are looking at spherical harmonics of order < 10 as is typical in acoustics. I don't think its necessary to change imho.

With the same logic we could make this "jit"-able, but it's a bit of an overkill.

mberz · 2025-03-06T18:06:18Z

spharpy/spherical.py

+        if channel_convention == "fuma":
+            order, degree = fuma_to_nm(acn)
+        else:
+            order, degree = acn_to_nm(acn)
        basis[:, acn] = _special.spherical_harmonic_real(
-            order,
-            degree,
-            coords.colatitude,
-            coords.azimuth)
+            order, degree, coordinates.colatitude, coordinates.azimuth
+        )
+        if normalization == "sn3d":
+            basis[:, acn] *= n3d_to_sn3d_norm(order)
+        elif normalization == "maxN":
+            basis[:, acn] *= n3d_to_maxn(acn)
+        if phase_convention is None:
+            # Condon-Shortley phase term is already included in
+            # the special.spherical_harmonic function
+            # so need to divide by (-1)^m
+            basis[:, acn] /= (-1) ** float(degree)


Same comment as above, if possible, please move the re-normalization and sorting out of the loop.

mberz · 2025-03-06T18:09:07Z

spharpy/spherical.py

+        if phase_convention is None:
+            # Condon-Shortley phase term is already included in
+            # the special.spherical_harmonic function
+            # so need to divide by (-1)^m
+            basis[:, acn] /= (-1) ** float(degree)


I think the spherical_harmonic_real function does not include the CS phase. Please check the issue I created on this and compare.
Technically, there's no difference between multiplying with (-1)^m or dividing by, but the comment is not correct then ;)

mberz · 2025-03-06T18:10:03Z

spharpy/spherical.py

+    if not isinstance(coordinates, pf.Coordinates):
+        axis = np.where(coordinates.shape == 3)[0][0]
+        if axis == 0:
+            coordinates = coordinates.T
+        coordinates = pf.Coordinates(coordinates[:, 0],
+                                     coordinates[:, 1],
+                                     coordinates[:, 2])


Same comment as above. I don't think we should do auto-transforming coordinates.,

Same as above. It is removed now.

mberz · 2025-03-06T18:11:50Z

tests/test_spherical_harmonics.py

+    Y = np.genfromtxt(f'./tests/data/Y_real_{phase_convention}_'
+                      f'{normalization}_{channel_convention}.csv',
+                      dtype=float,
+                      delimiter=',')
+    basis = sh.spherical_harmonic_basis_real(n_max, coords,
+                                             normalization,
+                                             channel_convention,
+                                             phase_convention=phase_convention)
+    np.testing.assert_allclose(basis, Y, atol=1e-13)


Could you also for the benefit of my sanity keep a test against the old files. Just so we're sure that we don't accidentally change the default normaltion and phase conventions of the files? ;)

Sure, I added test against your old n10 files for both, real and complex. However, the new default for Condon-Shortley is None, so we explicitly need to test against phase_convention='Condon-Shortley', not the default.. Probably we should discuss at this point if we want to have "Condon-Shortley" as the default, to be consistent with old implementations?

mberz · 2025-03-06T18:14:16Z

spharpy/spherical.py

    return basis


-def spherical_harmonic_basis_gradient(n_max, coords):
+def spherical_harmonic_basis_gradient(n_max, coordinates):


You did not implement the phase and normalization conventions for the gradients. Is there a reason for this?
Did you want to keep this for a separate PR?

No, just not thought about it :D. I will implement it in this branch.

mberz · 2025-03-06T18:15:30Z

spharpy/spherical.py

+    normalization : str, optional
+        Normalization convention, either 'n3d', 'maxN' or 'sn3d'.
+        The default is 'n3d'.
+        (maxN is only supported up to 3rd order)


Since this is only valid up to third order we should raise an error on invalid inputs.

mberz · 2025-03-06T18:15:35Z

spharpy/spherical.py

+    channel_convention : str, optional
+        Channel ordering convention, either 'acn' or 'fuma'.
+        The default is 'acn'.
+        (FuMa is only supported up to 3rd order)


Since this is only valid up to third order we should raise an error on invalid inputs.

…ns to sh gradient methods

Edit docstrings of sphercal_harmonics, spherical_harmonics_real Update reference data update tests

f-brinkmann · 2025-03-27T12:51:52Z

spharpy/spherical.py


-        Y_n^m(\theta, \phi) = \sqrt{\frac{2n+1}{4\pi}
-        \frac{(n-m)!}{(n+m)!}} P_n^m(\cos \theta) e^{i m \phi}
+    where:


The math below is not displayed correctly in the docs. Probably you need to use the :math: tag in Sphinx

f-brinkmann · 2025-03-27T12:53:33Z

spharpy/spherical.py

+    - $P_{nm}$ is the associated Legendre function
+    - $N_{nm}$ is the normalization term
+    - $CS_m$ is the Condon-Shortley phase term


Can we link somewhere for an analytical description of these? For the last, we could also write CS_m = (-1)^m directly here.

https://mathworld.wolfram.com/SphericalHarmonic.html

f-brinkmann

Found only a few minor things. I generally wonder where we give a complete analytical description of the SHs? Is that given in the pull related to the SH-class?

ahms5

Thank you for taking care! just checked the docs

ahms5 · 2025-03-27T13:01:44Z

spharpy/special.py

@@ -159,6 +159,8 @@ def _spherical_hankel_derivative(n, z, kind):

 def spherical_harmonic(n, m, theta, phi):
    """The spherical harmonics of order n and degree m.
+    The spherical harmonic functions are fully normalized (N3D) and


Suggested change

The spherical harmonic functions are fully normalized (N3D) and

The spherical harmonic functions are fully normalized (N3D) and

very picky suggestion^^

ahms5 · 2025-03-27T13:04:27Z

spharpy/special.py

+    the AmbiX phase convention which does not include the Condon-Shortley
+    phase [#]_.


Suggested change

the AmbiX phase convention which does not include the Condon-Shortley

phase [#]_.

the AmbiX phase convention, which does not include the Condon-Shortley

phase [#]_.

ahms5 · 2025-03-27T13:07:34Z

spharpy/spherical.py

-
-    The spherical harmonic functions are fully normalized (N3D) and include the
-    Condon-Shotley phase term :math:`(-1)^m` [#]_, [#]_.
+    See also :func:`spherical_harmonic_basis_real`.


Suggested change

See also :func:`spherical_harmonic_basis_real`.

See [#]_ and [#]_ for details.

See also :py:func:`spherical_harmonic_basis_real`.

we need to refer the references, otherwise the doc build on ci will fail

ahms5 · 2025-03-27T13:18:23Z

spharpy/spherical.py

-        Y_n^m(\theta, \phi) = \sqrt{\frac{2n+1}{4\pi}
-        \frac{(n-m)!}{(n+m)!}} P_n^m(\cos \theta) e^{i m \phi}
+    where:
+    - $n$ is the degree


Suggested change

- $n$ is the degree

- :math:`n` is the degree

like this, the empty line should also fix the bulletpoints

ahms5 · 2025-03-27T13:24:16Z

spharpy/spherical.py

+        (FuMa is only supported up to 3rd order)
+    phase_convention : string or None, optional
+        Whether to include the Condon-Shortley phase term.
+        The default is 'Condon-Shortley'.


we should test if no other string than 'Condon-Shortley' is used

ahms5 · 2025-03-27T13:32:25Z

spharpy/spherical.py

-
-    The spherical harmonic functions are fully normalized (N3D) and follow
-    the AmbiX phase convention [#]_.
+    See also :func:`spherical_harmonic_basis`.


Suggested change

See also :func:`spherical_harmonic_basis`.

See also :py:func:`spherical_harmonic_basis`.

ahms5 · 2025-03-27T13:34:46Z

spharpy/spherical.py

+        (FuMa is only supported up to 3rd order)
+    phase_convention : string or None, optional
+        Whether to include the Condon-Shortley phase term.
+        The default is None.


does it make sence to have different defaults for the "same" parameter?
we should also highlight the possible opitons.
if we wouldnt add more options in future we could also think about a boolean input ike apply_cs_phase

ahms5 · 2025-03-27T13:34:54Z

spharpy/spherical.py

@@ -471,9 +567,21 @@ def spherical_harmonic_basis_gradient_real(n_max, coords):
    ----------
    n_max : int
        Spherical harmonic order
-    coordinates : :doc:`pf.Coordinates <pyfar:classes/pyfar.coordinates>`
-        Coordinate object with sampling points for which the basis matrix is
+    coordinates : :doc:`pf.Coordinates <pyfar:classes/pyfar.coordinates>` or


ahms5 · 2025-03-27T13:35:09Z

spharpy/spherical.py

+def spherical_harmonic_basis_gradient_real(n_max, coordinates,
+                                           normalization="n3d",
+                                           channel_convention="acn",
+                                           phase_convention=None):
    r"""
    Calulcates the unit sphere gradients of the real valued spherical hamonics.


Calculates harmonics*

ahms5 · 2025-03-27T13:35:19Z

spharpy/spherical.py

+        (FuMa is only supported up to 3rd order)
+    phase_convention : string or None, optional
+        Whether to include the Condon-Shortley phase term.
+        The default is None.


Adapt SH basis matrix + docstring and type checking

78aeba2

Co-authored-by: Xenofon Karakonstantis <[email protected]>

mberz assigned xefonon Feb 21, 2025

mberz added the enhancement New feature or request label Feb 21, 2025

mberz added this to the v1.0.0 milestone Feb 21, 2025

This was referenced Feb 21, 2025

Dev spherical harmonic class #82

Closed

Add Spherical Harmonics class #101

Open

tluebeck added 6 commits February 25, 2025 21:38

bugfix negative powers

26c7f80

add condon shortley to real valued basis functions, add phase convent…

5d3c996

…ion to tests

bugfix n3d_to_sn3d_norm, add test for normalization and channel conve…

44517c2

…ntion

implement tests for real and complex sh bases

858a73d

add reference SH bases as csv

fix n10 tests

985db91

fix wigner_d tests

34b7fba

tluebeck requested review from f-brinkmann and ahms5 March 5, 2025 20:48

mberz commented Mar 6, 2025

View reviewed changes

mberz assigned tluebeck Mar 6, 2025

tluebeck added 8 commits March 7, 2025 07:57

update according to review

d19942c

add tests for invalid maxN and FuMa

ae2ec44

add different normalization, channel conventions, and phase conventio…

10005b0

…ns to sh gradient methods

temporary change default phase_convention for consistency to old tests

a0ba1ae

update according to review

80c12c9

Edit docstrings of sphercal_harmonics, spherical_harmonics_real Update reference data update tests

change defaults of gradients

4958871

edit docs in tests

ccdb77d

add tests for sh gradients

526878e

mberz unassigned xefonon Mar 14, 2025

mberz requested a review from xefonon March 14, 2025 13:34

f-brinkmann reviewed Mar 27, 2025

View reviewed changes

f-brinkmann requested changes Mar 27, 2025

View reviewed changes

ahms5 reviewed Mar 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement different normalization and phase conventions for spherical harmonic basis matrices #102

Implement different normalization and phase conventions for spherical harmonic basis matrices #102

mberz commented Feb 20, 2025

mberz left a comment

mberz Mar 6, 2025

tluebeck Mar 7, 2025

mberz Mar 6, 2025

f-brinkmann Mar 27, 2025 •

edited

Loading

xefonon Mar 27, 2025

xefonon Mar 27, 2025

mberz Mar 6, 2025

mberz Mar 6, 2025

mberz Mar 6, 2025

tluebeck Mar 7, 2025

mberz Mar 6, 2025

tluebeck Mar 7, 2025

mberz Mar 6, 2025

tluebeck Mar 7, 2025

mberz Mar 6, 2025

tluebeck Mar 7, 2025

mberz Mar 6, 2025

tluebeck Mar 7, 2025

f-brinkmann Mar 27, 2025 •

edited

Loading

f-brinkmann Mar 27, 2025

xefonon Mar 27, 2025

f-brinkmann left a comment

ahms5 left a comment

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

ahms5 Mar 27, 2025

	The spherical harmonic functions are fully normalized (N3D) and

	The spherical harmonic functions are fully normalized (N3D) and

		the AmbiX phase convention which does not include the Condon-Shortley
		phase [#]_.

-    See also :func:`spherical_harmonic_basis_real`.
+    See [#]_ and [#]_ for details.
+    See also :py:func:`spherical_harmonic_basis_real`.

	See also :func:`spherical_harmonic_basis`.
	See also :py:func:`spherical_harmonic_basis`.

Implement different normalization and phase conventions for spherical harmonic basis matrices #102

Are you sure you want to change the base?

Implement different normalization and phase conventions for spherical harmonic basis matrices #102

Conversation

mberz commented Feb 20, 2025

mberz 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

f-brinkmann Mar 27, 2025 • edited Loading

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

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 Mar 27, 2025 • edited Loading

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

ahms5 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-brinkmann Mar 27, 2025 •

edited

Loading

f-brinkmann Mar 27, 2025 •

edited

Loading