Introduce property sh_caxis#327
Conversation
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
@tluebeck I think we have to differentiate between caxis and axis. The SH audio objects should deal with the sh_caxis and the default should be -1. Functions that work on the SH data (numpy array) directly, such as _assert_valud_number_of_sh_channels must get the sh_axis = sh_caxis - 1. Do you agree or did I miss something?
At some point, we should add a check if sh_caxis<0 and np.abs(sh_caxis)<data.cdim
|
I agree, but I'm not sure if we want to pass "sh_axis" to SHAudio object's inits and internally recalculate to sh_caxis or if we should let the user pass "sh_caxis" directly. |
|
I decided to pass sh_caxis directly. |
f-brinkmann
left a comment
f-brinkmann
left a comment
There was a problem hiding this comment.
Thanks for changing. I think I found one thing that needs further adjustment and had only comments on the docs apart from that.
| Specifies which axis of data holds the spherical harmonic coefficients. | ||
| Negative indexing, i.e., interpreted relative to the end of the array. | ||
| default is second last axis (-2). | ||
| Specifies which channel axis of data holds the spherical harmonic |
There was a problem hiding this comment.
We could additionally link
to geive background on the caxis conept, as we do in the pyfar documentaiton.
| Negative indexing, i.e., interpreted relative to the end of the array. | ||
| default is second last axis (-2). | ||
| Specifies which channel axis of data holds the spherical harmonic | ||
| coefficients. Negative indexing, i.e., interpreted relative to the end |
There was a problem hiding this comment.
Could be more clear, that this is required. maybe:
'Must be an integer smaller than or equal to -1. The default -1 referrs to the last channel axis and a value of -2 would refer to the second last channel axis.'
| if abs(sh_caxis) >= data.ndim: | ||
| raise ValueError(f"sh_caxis ({sh_caxis}) exceeds the number of " | ||
| f"dimensions of data ({data.ndim})") |
There was a problem hiding this comment.
Should we move the sh_caxis checks to the end of the initializationWe could then useabs(sh_caxis) >= self.cdimotherwise I think we also have to testabs(sh_caxis) >= data.ndim + 1`.
There was a problem hiding this comment.
We should check that before data = _atleast_3d_first_dimension(data) and _assert_valid_number_of_sh_channels(data.shape, sh_caxis-1), otherwise it crashes.
| if abs(sh_caxis) >= data.ndim: | ||
| raise ValueError(f"sh_caxis ({sh_caxis}) exceeds the number of " | ||
| f"dimensions of data ({data.ndim})") | ||
|
|
There was a problem hiding this comment.
See comment above (cdim or ndim + 1)
Adapt docstrings
mberz
left a comment
mberz
left a comment
There was a problem hiding this comment.
I've added some comments below.
Additionally, I have another comment:
In one of the last meetings, we discussed support for multiple dimensions containing spherical harmonic coefficients. In this implementation this is not possible, correct?
Or it simply would not be covered, but users would need to add the data in an additional dimension which is unaffected by all checks.
Would you think it's feasible to implement this as well?
| import numpy as np | ||
| import numpy.testing as npt | ||
| import pytest | ||
| import re |
| def sh_caxis(self): | ||
| """Get the spherical harmonic axis""" | ||
| return self._sh_caxis |
There was a problem hiding this comment.
| def sh_caxis(self): | |
| """Get the spherical harmonic axis""" | |
| return self._sh_caxis | |
| def caxis_spherical_harmonics(self): | |
| """Get the spherical harmonic axis""" | |
| return self._sh_caxis |
I've thought about it a bit and came to the conclusion that I'd prefer a naming convention where additional info is appended instead of prepended. In my opinion that makes things a bit clearer. (also related concepts are closed together in terms of auto-complete, documentation, etc.
There was a problem hiding this comment.
Sounds reasonable to me.
| if sh_caxis > 0: | ||
| raise ValueError("sh_caxis has to be a negative integer.") |
There was a problem hiding this comment.
I don't think that's a necessary condition. It's just a good default to start from the end.
change property name adapt tests
3 participants
Which issue(s) are closed by this pull request?
Closes #326
Changes proposed in this pull request: