Adapt Rotation to the SphericalHarmonicDefintion class #226

mberz · 2025-11-19T17:25:43Z

Changes proposed in this pull request:

Rename RotationSH to SphericalHarmonicDefintion
Use SphericalHarmonicDefintion instead of n_max
allow applying to SphericalHarmonic* data objects
overload multiplication operator
update and extend documentation

Requires

Copilot

Pull request overview

This pull request refactors the rotation functionality to use the SphericalHarmonicDefinition class for defining spherical harmonic properties instead of passing n_max as a parameter.

Renamed RotationSH class to SphericalHarmonicRotation for better naming consistency
Refactored rotation matrix generation to use SphericalHarmonicDefinition objects instead of separate n_max and basis_type parameters
Updated rotation methods to work with SphericalHarmonicSignal objects via duck typing

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File	Description
spharpy/transforms/rotations.py	Renamed `RotationSH` to `SphericalHarmonicRotation`, refactored to use `SphericalHarmonicDefinition` for rotation matrix creation, simplified class by removing explicit classmethod overrides, added `__mul__` operator for rotation composition and signal rotation
spharpy/transforms/init.py	Updated exports to use new `SphericalHarmonicRotation` class name
tests/test_rotations.py	Updated tests to use new class name and API, added new tests for `apply` method and rotation multiplication operator

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

spharpy/transforms/rotations.py

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

spharpy/transforms/rotations.py

tests/test_rotations.py

spharpy/transforms/rotations.py

f-brinkmann

Thanks! Conceptually everything is fine to me. I left a few comments on the docs already but did not review the tests, yet.

spharpy/transforms/rotations.py

…ericalHarmonicRotation class

…onicRotation class methods

f-brinkmann

Thanks, this is great to have! Code looks good to me. I left some comments on how the docs might be improved and have thought for discussion:

We have to ways to apply a rotation to a SHAudio object and it might be a little confusing that * is used instead of @. Do we want to keep both ways? Do we want to use * or doe we want / could we use @?

I would suggest to open an issue to move this to the classes module. In this case

the example in the docstring of the class could be moved above the class
In the module docstring of the transforms module, I would suggest to mention, that these are helper functions and that we recomment to use the SHRotation class

f-brinkmann · 2025-12-19T12:07:19Z

spharpy/transforms/rotations.py

+class SphericalHarmonicRotation(ScipyRotation):
+    """Class for rotations of coordinates and spherical harmonic expansions.
+
+    The class extends the :py:class:`~scipy.spatial.transform.Rotation` class


Suggested change

The class extends the :py:class:`~scipy.spatial.transform.Rotation` class

The class extends the SciPy :py:class:`~scipy.spatial.transform.Rotation` class

f-brinkmann · 2025-12-19T12:08:18Z

spharpy/transforms/rotations.py

+    .. [#] B. Rafaely, Fundamentals of Spherical Array Processing, 1st ed.,
+           vol. 8. Springer-Verlag GmbH Berlin Heidelberg, 2015.


Should cite the second edition:

Suggested change

.. [#] B. Rafaely, Fundamentals of Spherical Array Processing, 1st ed.,

vol. 8. Springer-Verlag GmbH Berlin Heidelberg, 2015.

.. [#] B. Rafaely, Fundamentals of Spherical Array Processing, 2nd ed.,

vol. 8. Springer-Verlag GmbH Berlin Heidelberg, 2019.

f-brinkmann · 2025-12-19T12:10:46Z

spharpy/transforms/rotations.py

+    To create a rotation object, use one of the available
+    :py:meth:`~scipy.spatial.transform.Rotation.from_*` methods, e.g.,
+    :py:meth:`~scipy.spatial.transform.Rotation.from_euler` or
+    :py:meth:`~scipy.spatial.transform.Rotation.from_quat` for creation using
+    Euler angles or quaternions, respectively. Also see the examples below.


Since the examples follow directly below. I would suggest to delete this paragraph.

f-brinkmann · 2025-12-19T12:12:41Z

spharpy/transforms/rotations.py

+        >>>     'z', angle)
+
+        The spherical harmonic rotation matrix can be obtained and applied
+        to the using the following:


Suggested change

to the using the following:

to the coefficients using the following:

f-brinkmann · 2025-12-19T12:16:14Z

spharpy/transforms/rotations.py

+        >>>     definition, coordinates=sampling)
+        ...
+        >>> _, axs = plt.subplots(
+        >>>     1, 2, subplot_kw={'projection': '3d'}, figsize=(5, 2.5))


The axis labels are partly overlapping and clipped. Can you try to improve this, e.g., by adding space or increasing figure size?

f-brinkmann · 2025-12-19T12:24:06Z

spharpy/transforms/rotations.py

+        The effect of the rotation can again be visualized by evaluating the
+        series expansion on the unit sphere:
+
+        >>> _, axs = plt.subplots(


See comment above on overlapping/clipped labels.

f-brinkmann · 2025-12-19T12:32:45Z

spharpy/transforms/rotations.py

+        The rotation can now be applied using the `*` operator:
+        Note that due to the multiplication with itself, the rotation
+        is applied twice, which corresponds to a rotation of 90 degrees around
+        the z-axis.
+
+        >>> rotated_frequency_data = R * R * frequency_data


I would suggest to show all ways in which a rotation can be applied in one spot. This would shorten the example and bundle information in one spot. For simplicity, I would only apply one rotation (and maybe make it 90 degrees, because the plot is more clear in this case)

Suggested change

The rotation can now be applied using the `*` operator:

Note that due to the multiplication with itself, the rotation

is applied twice, which corresponds to a rotation of 90 degrees around

the z-axis.

>>> rotated_frequency_data = R * R * frequency_data

The rotation can be applied in different ways. Mathematically it is applied

by a matrix multiplication of the roation matrix with the spherical harmonic

coefficients

>>> rotated_coefficients = R @ coefficients

This is the same as using the coefficients store in the spherical harmonic

FrequencyData obejct

>>> rotated_coefficients = R @ frequency_data.freq

However, it might be more convenient to keep the coefficients inside the

FrequencyData object and apply the roation using the `*` operator

operator

>>> rotated_frequency_data = R * frequency_data

which can also be done using the `apply` method

>>> rotated_frequency_data = R.apply(frequency_data)

f-brinkmann · 2025-12-19T12:39:36Z

spharpy/transforms/rotations.py

+        """Apply the rotation to spherical harmonic data object.
+
+        Note that the spherical harmonic definition of the target object is
+        used to generate a fitting spherical harmonic rotation matrix.


Misses

Note ---- At the moment, only spherical harmonic rotations matrices following the ``ACN`` indexing and ``N3D`` normalization are supported.

f-brinkmann · 2025-12-19T12:41:17Z

spharpy/transforms/rotations.py

-        return M @ coefficients
+            raise ValueError(
+                "Multiplication is only supported for "
+                "SphericalHarmonicRotation and SphericalHarmonic* audio "


I think it is clear withtou '*'

Suggested change

"SphericalHarmonicRotation and SphericalHarmonic* audio "

"SphericalHarmonicRotation and SphericalHarmonic audio "

f-brinkmann · 2025-12-19T12:48:54Z

tests/test_rotations.py

Can you please add a brief dosctring to each test?

mberz added this to the v1.0.0 milestone Nov 19, 2025

mberz self-assigned this Nov 19, 2025

mberz added the refactoring label Nov 19, 2025

mberz added this to Weekly Planning Nov 19, 2025

github-project-automation bot moved this to Backlog in Weekly Planning Nov 19, 2025

mberz moved this from Backlog to Implementation in progress in Weekly Planning Nov 19, 2025

mberz requested a review from Copilot November 28, 2025 16:06

Copilot started reviewing on behalf of mberz November 28, 2025 16:07 View session

Copilot finished reviewing on behalf of mberz November 28, 2025 16:10

Copilot AI reviewed Nov 28, 2025

View reviewed changes

mberz requested a review from Copilot November 28, 2025 16:38

Copilot started reviewing on behalf of mberz November 28, 2025 16:39 View session

Copilot finished reviewing on behalf of mberz November 28, 2025 16:40

Copilot AI reviewed Nov 28, 2025

View reviewed changes

spharpy/transforms/rotations.py Outdated Show resolved Hide resolved

spharpy/transforms/rotations.py Show resolved Hide resolved

tests/test_rotations.py Outdated Show resolved Hide resolved

spharpy/transforms/rotations.py Show resolved Hide resolved

f-brinkmann reviewed Dec 12, 2025

View reviewed changes

mberz force-pushed the refactor/rotation_adapt_sh_definition branch 2 times, most recently from ad94866 to 345c174 Compare December 17, 2025 07:35

mberz added 13 commits December 17, 2025 08:37

initial draft for adapting the SphericalHarmonicDefinition in the Sph…

5f49a6b

…ericalHarmonicRotation class

update init to renaming of the rotation class

cc44989

update test to new rotation class name

83651c3

update test functionality

46644bf

Use default SphericalHarmonicDefinition for init of the SphericalHarm…

7d0bf34

…onicRotation class methods

Avoid overloading each from_ and as_ class method

d629304

initial versions of the apply method and __mul__ operator.

d2810c4

Simple tests

9d4bbfc

fix intentation

c1ddaae

update docstrings

217d650

bugfix stack of matrices

e2b5cb5

raise error for unsupported multiplication types

7859125

Update new spherical harmonic audio types

8ac6813

mberz added 2 commits December 17, 2025 08:37

formatting

3405b6f

Docstring updates

522bafb

mberz force-pushed the refactor/rotation_adapt_sh_definition branch from 345c174 to 522bafb Compare December 17, 2025 07:37

mberz added 9 commits December 17, 2025 09:22

Update docstring example

590e2c9

Allow all spherical harmonic audio container classes

f22634f

Add context

80a0b02

raise exceptions for not implemented definitions

020b6b3

Use standardized convention for rotation of audio objects

4d4da02

update error message

9460b6c

update docstring

d8cbbf2

exclude __init__ from docs

8fb1f12

update docstring

61631ab

mberz marked this pull request as ready for review December 17, 2025 16:41

ruff fixes

5e4cb3d

mberz requested review from a team, ahms5, f-brinkmann, hoyer-a, sikersten and tluebeck December 18, 2025 18:26

f-brinkmann reviewed Dec 19, 2025

View reviewed changes

	The class extends the :py:class:`~scipy.spatial.transform.Rotation` class
	The class extends the SciPy :py:class:`~scipy.spatial.transform.Rotation` class

		.. [#] B. Rafaely, Fundamentals of Spherical Array Processing, 1st ed.,
		vol. 8. Springer-Verlag GmbH Berlin Heidelberg, 2015.

	to the using the following:
	to the coefficients using the following:

-        The rotation can now be applied using the `*` operator:
-        Note that due to the multiplication with itself, the rotation
-        is applied twice, which corresponds to a rotation of 90 degrees around
-        the z-axis.
-        >>> rotated_frequency_data = R * R * frequency_data
+        The rotation can be applied in different ways. Mathematically it is applied
+        by a matrix multiplication of the roation matrix with the spherical harmonic
+        coefficients
+        >>> rotated_coefficients = R @ coefficients
+        This is the same as using the coefficients store in the spherical harmonic
+        FrequencyData obejct
+        >>> rotated_coefficients = R @ frequency_data.freq
+        However, it might be more convenient to keep the coefficients inside the
+        FrequencyData object and apply the roation using the `*` operator
+        operator
+        >>> rotated_frequency_data = R * frequency_data
+        which can also be done using the `apply` method
+        >>> rotated_frequency_data = R.apply(frequency_data)

	"SphericalHarmonicRotation and SphericalHarmonic* audio "
	"SphericalHarmonicRotation and SphericalHarmonic audio "

Adapt Rotation to the SphericalHarmonicDefintion class #226

Are you sure you want to change the base?

Adapt Rotation to the SphericalHarmonicDefintion class #226

Uh oh!

Conversation

mberz commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request:

Requires

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

f-brinkmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mberz commented Nov 19, 2025 •

edited

Loading