Spherical harmonic class addition #59
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.
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.
| from spharpy.samplings.helpers import calculate_sampling_weights | ||
|
|
||
|
|
||
| class SphHarm: |
There was a problem hiding this comment.
Should we be more explicit and call it SphericalHarmonics?
There was a problem hiding this comment.
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.
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.
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.
Would it make sense to mention already here, the dimension of the SH matrix?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
| self.compute_inverse() | ||
| return self._basis_inv | ||
|
|
||
| def compute_inverse(self, inv_transform_type=None, weights=None): |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
| from spharpy.samplings.helpers import calculate_sampling_weights | ||
|
|
||
|
|
||
| class SphHarm: |
There was a problem hiding this comment.
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.
| 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.
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): |
| Normalization convention, either 'n3d', 'maxN' or 'sn3d'. The default is 'n3d'. | ||
| (maxN is only supported up to 3rd order) | ||
| """ | ||
| self.n_max = n_max |
| self.compute_inverse() | ||
| return self._basis_inv | ||
|
|
||
| def compute_inverse(self, inv_transform_type=None, weights=None): |
There was a problem hiding this comment.
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.
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.
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