New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spherical harmonic class addition #59
base: develop
Are you sure you want to change the base?
Conversation
Normalization convention, either 'n3d', 'maxN' or 'sn3d'. The default is 'n3d'. | ||
(maxN is only supported up to 3rd order) | ||
""" | ||
self.n_max = n_max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to turn n_max
and all other arguments into properties having a getter and setter. In this case self.n_max = n_max
would call the setter, which would be the best place for performing the check that is currently done in _validate_parameters
. Because self.n_max
could not be used as a variable anymore, we would typically store the data in self._n_max
indicating it as private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
from spharpy.samplings.helpers import calculate_sampling_weights | ||
|
||
|
||
class SphHarm: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be more explicit and call it SphericalHarmonics
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, during the Coordinates labeling discussion we decided not to use stort names.
class SphHarm: | ||
def __init__(self, n_max, coords, basis_type='complex', channel_convention='acn', inv_transform_type=None, | ||
normalization='n3d'): | ||
r""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically move the docstring before the __init__
. This way it shows up as the class documentation and we do not need to include __init__
in the documentation, which might be looking weird for users with little Python experience.
def __init__(self, n_max, coords, basis_type='complex', channel_convention='acn', inv_transform_type=None, | ||
normalization='n3d'): | ||
r""" | ||
Create a spherical harmonics class for transformation between spherical harmonics and signals. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
Create a spherical harmonics class for transformation between spherical harmonics and signals. | |
Class for spherical harmonics basis functions. | |
Objects of this class can be used to obtain spherical harminics basis functions, their inverse, and ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to mention already here, the dimension of the SH matrix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about giving the exact definitions of all Spherical Harmonics Conventions that can be realized with the class at the beginning of the docstring?
---------- | ||
n_max : int, | ||
Maximum spherical harmonic order | ||
coords : :doc:`pf.Coordinates <pyfar:classes/pyfar.coordinates>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the inverse if 'quadrature' the coordinates must have weights. This should be mentioned. Do we need spharpy.SamplingSphere
in this case instead of pyfar.Coordinates
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the spharpy.SamplingSphere
already contains an n_max, maybe we can use this n_max
. If we dont use the spharpy.SamplingSphere
here, then why do we need it?
Coordinate object with sampling points for which the basis matrix is | ||
calculated | ||
basis_type : str, optional | ||
Type of spherical harmonic basis, either 'complex' or 'real'. The default is 'complex'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some anyoing docstring conventions: If we refer to a parameter, we make it appear itallic. This is done by single ` quotes. If we use code we make it appear as coode e.g. ``'complex'``
calculated | ||
basis_type : str, optional | ||
Type of spherical harmonic basis, either 'complex' or 'real'. The default is 'complex'. | ||
channel_convention : str, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a bit more detailed and explain the channel ordering for both cases
channel_convention : str, optional | ||
Channel ordering convention, either 'acn' or 'fuma'. The default is 'acn'. | ||
(FuMa is only supported up to 3rd order) | ||
inv_transform_type : str, optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think some more detail would help also here, i.e., briefly mention how the inverse is gerneated in both cases.
else: | ||
raise ValueError("Invalid signal type, should be either 'complex' or 'real'") | ||
|
||
def compute_basis(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be something taken care of by the class? If the bases is requested, pass it if it is already computed and up to data or re-compute it otherwise. If yes, I think this could be private and we could only have a property basis
as a getter. This would also affect the other compute_*
functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort. I mainly left concpetual questions for discussing in the group. And I wondered if we have functions for returning the index of a specific order n and degree m inside a SH matrix? That might come in handy in some cases.
self.compute_inverse() | ||
return self._basis_inv | ||
|
||
def compute_inverse(self, inv_transform_type=None, weights=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everything would be realized by setter and getters, the inv_transform
and weights
could be removed here. The latter would have to be inluded in the Coordinates of SamplingSphere object stored as part of the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the weights could be taken from the spharpy.SamplingSphere
object.
self._basis_inv = np.conj(self.basis).T * (weights) | ||
|
||
@property | ||
def basis_inv_gradient_theta(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more of a general question for discussion: Should we use the names of the angles according to pyfar? It would be colatitude and azimuth in this case.
self._basis_inv_gradient_theta = np.conj(self.basis_gradient_theta).T * (4 * np.pi * weights) | ||
self._basis_inv_gradient_phi = np.conj(self.basis_gradient_phi).T * (4 * np.pi * weights) | ||
|
||
def transform(self, signal): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another general question: Should the transform functions below be part of the SH class or would they go somewhere else? I guess with the inverse defined, they could be part. But aren't there cases where radial functions are combined with the SH matrix before the transform?
# TODO: Implement filtering | ||
pass | ||
|
||
def plot_basis_functions(self, order, mode): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the pyfar logic this should go to a spharpy.plot
module. We still have a plot function in the Coordinates class, but also want to (re)move it there. Maybe we can exclude it here an open an issue to make sure not to forget.
# TODO: Implement plotting function for basis functions | ||
pass | ||
|
||
def plot_reconstructed_signal(self, coefficients): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require a dedictaed plotting function or could pyfar.plot
be used for this?
pass | ||
|
||
|
||
def to_maxN_norm(acn): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also discuss, if we want to move the functions belwo into the SH class or if they are usefull outside of it as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the effort. I mainly left concpetual questions for discussing in the group. And I wondered if we have functions for returning the index of a specific order n and degree m inside a SH matrix? That might come in handy in some cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing, maybe we should also discuss to move this to shparpy/classes/SphericalHarmonics.py, and bring it to toplevl. see pyfar.
from spharpy.samplings.helpers import calculate_sampling_weights | ||
|
||
|
||
class SphHarm: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, during the Coordinates labeling discussion we decided not to use stort names.
@@ -1,6 +1,431 @@ | |||
import numpy as np | |||
import scipy.special as special | |||
import spharpy.special as _special | |||
import logging as logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import logging as logger |
---------- | ||
n_max : int, | ||
Maximum spherical harmonic order | ||
coords : :doc:`pf.Coordinates <pyfar:classes/pyfar.coordinates>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the spharpy.SamplingSphere
already contains an n_max, maybe we can use this n_max
. If we dont use the spharpy.SamplingSphere
here, then why do we need it?
else: | ||
raise ValueError("Invalid signal type, should be either 'complex' or 'real'") | ||
|
||
def compute_basis(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
Normalization convention, either 'n3d', 'maxN' or 'sn3d'. The default is 'n3d'. | ||
(maxN is only supported up to 3rd order) | ||
""" | ||
self.n_max = n_max |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed
self.compute_inverse() | ||
return self._basis_inv | ||
|
||
def compute_inverse(self, inv_transform_type=None, weights=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the weights could be taken from the spharpy.SamplingSphere
object.
self._basis_inv_gradient_phi = np.linalg.pinv(self.basis_gradient_phi) | ||
elif self.inv_transform_type == 'quadrature': | ||
if weights is None: | ||
weights = calculate_sampling_weights(self.coords) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe during setting/init the spharpy.SamplingSphere
, we could check if it contains weigths
and n_max
, otherwise raise an error. Then the user need to take care of it.
self.compute_inverse() | ||
return np.dot(self.basis_inv.T, coefficients) | ||
|
||
def inverse_transform_gradient(self, coefficients): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sence to replace this runctions by oberoading *
or sth. like this?
Changed SphericalHarmonics class after initial review Add unit tests for SphericalHarmonics class in spharpy
Hello all,
I've drafted the spherical harmonics class "Sphharm" with some features already included and potential for extension.
Feedback is appreciated.
Best,
Xenofon