Spherical harmonic class addition #59
Conversation
Changed SphericalHarmonics class after initial review Add unit tests for SphericalHarmonics class in spharpy
…calculation Added SphericalHarmonics class with methods for computing spherical harmonic basis matrices, their gradients, and handling various spherical harmonic operations. Also included helper functions for spherical harmonic transformations and calculations
Still requires coordination for Ambisonics class
spharpy/spherical.py
Outdated
| Calculate the linear index coefficient for a order n and degree m, | ||
| Calculate the spherical harmonic order n and degree m for a linear | ||
| coefficient index, according to the FuMa (Furse-Malham) | ||
| Channel Ordering Convention [2]_. |
There was a problem hiding this comment.
This will automatically number the references, which is safer
| Channel Ordering Convention [2]_. | |
| Channel Ordering Convention [#]_. |
|
Hello, thanks for the review so far. I've tried to incorporate most of the comments in the revised code. Let me know what you think, and I can fix the CircleCI tests also. |
There was a problem hiding this comment.
My comments are only on the SH class implementation.
They're mostly on slimming down the class and making it a bit more accessible by removing redundancy.
One thing I'm not sure is the calculation of the inverse of the gradient. I'd argue that it's too specific and should not be part of the class, but implemented as part of code which does rely on it. I wasn't sure for example if a valid orthogonal basis relying on quadrature does also results in orthogonal transforms when considering the respective gradients.
The diff of this file is still quite messy, which makes it hard to track the changes. I've tried to clean up the diff a bit in #76.
I think it would be useful to merge #76 into this branch and re-write the commit history before merging into main once this PR is finalised.
Please turn off auto-formatting or similar when editing the file.
spharpy/spherical.py
Outdated
| def compute_basis_gradient(self): | ||
| try: | ||
| return self._compute_basis_gradient() | ||
| except Exception as e: | ||
| raise ValueError("Error computing basis gradient:", e) from e |
There was a problem hiding this comment.
| def compute_basis_gradient(self): | |
| try: | |
| return self._compute_basis_gradient() | |
| except Exception as e: | |
| raise ValueError("Error computing basis gradient:", e) from e |
As with the other public calculation methods, I'd remove this and only keep the private ones.
spharpy/spherical.py
Outdated
| def condon_shortley(self, value): | ||
| if not isinstance(value, bool): | ||
| raise TypeError("condon_shortley must be a boolean value") | ||
| if value != self._condon_shortley: | ||
| self._recompute_basis = True | ||
| self._recompute_basis_gradient = True | ||
| self._recompute_inverse = True | ||
| self._recompute_inverse_gradient = True | ||
| self._condon_shortley = value |
There was a problem hiding this comment.
As far as I remember the Condon Shortley phase is only used for complex valued SHs
spharpy/spherical.py
Outdated
| self._prev_n_max = self.n_max | ||
| self._prev_coordinates = self.coordinates | ||
| self._prev_basis_type = self.basis_type | ||
| self._prev_inverse_transform = self.inverse_transform | ||
| self._prev_channel_convention = self.channel_convention | ||
| self._prev_normalization = self.normalization | ||
| self._prev_condon_shortley = self.condon_shortley |
There was a problem hiding this comment.
| self._prev_n_max = self.n_max | |
| self._prev_coordinates = self.coordinates | |
| self._prev_basis_type = self.basis_type | |
| self._prev_inverse_transform = self.inverse_transform | |
| self._prev_channel_convention = self.channel_convention | |
| self._prev_normalization = self.normalization | |
| self._prev_condon_shortley = self.condon_shortley |
spharpy/spherical.py
Outdated
| channel_convention="acn", | ||
| inverse_transform=None, | ||
| weights=None, | ||
| condon_shortley=True |
There was a problem hiding this comment.
| condon_shortley=True | |
| condon_shortley=None |
I think this is conditional wheter the SHs are real or complex valued. I'm not aware of a real valued SH implementation using the CS phase.
Revert unrelated changes in SphericalHarmonic class branch
…eights (currently 2 ways of setting weights)
…in tests and rotations
…eights (currently 2 ways of setting weights)
Hello all,
I've drafted the spherical harmonics class "Sphharm" with some features already included and potential for extension.
Feedback is appreciated.
For discussion
cdim==1- raise error: has to be 1D (possible feature)Major:
ToDo
Comments Fabian
A few thoughts on the converters
n3d_to_maxnandn3d_to_sn3d_normchange n_sh back to n_max and make n_max docstring more explicit
For discussion: The names are inconsistent and should be changed. Were using '2' instead of 'to' in pyfar. Do we want to be consistent across packages here and use '2' everywhere?
A few notes on the docstrings:
Meeting 08/11/24: