NFC-HOA second-order-section filters #45
Conversation
sfs/time/drivingfunction.py
Outdated
|
|
||
| def _max_order_circular_harmonics(N, max_order): | ||
| """Compute order of 2D HOA.""" | ||
| return (N-1) // 2 if max_order is None else max_order |
There was a problem hiding this comment.
This is one of the functions we need for the time domain and the mono-frequent implementation. It is already implemented mono/drivingfunction.py, but in a slight different way:
def _max_order_circular_harmonics(N, max_order):
"""Compute order of 2D HOA."""
return N // 2 if max_order is None else max_orderHaving a look at Ahrens (2012), p. 132 there should also be a distinction of cases for even and odd numbers:
/ N/2 - 1, even nls
M = <
\ (N-1)/2 odd nls
Another problem is that this only holds for 2D and 2.5D. For 3D we would have:
_____
M = \|nls/2
see also nfchoa_order.m.
As we don't have 3D at the moment we could stick with the function name and change it later if we need 3D.
TODOS:
- find a common place and define only one
_max_order_circular_harmonics(N, max_order)function - add distinction between odd and even N
- add a reference to the help message
There was a problem hiding this comment.
Hi @narahahn,
I just realized that (N-1) // 2 is already the correct implementation of the max order function.
And the 2D vs. 3D problem can be solved by naming the 3D case _max_order_spherical_harmonics.
From the mentioned TODOs, then only these remain:
- find a common place and define only one
_max_order_circular_harmonics(N, max_order)function - add a reference to the help message
There was a problem hiding this comment.
I agree. Maybe in sfs.util?
There was a problem hiding this comment.
Yes, I think sfs.util would be currently the best fit.
While you are at it, you could replace the if/else expression with an if/else statement, I think this would be easier to understand.
Then it should also become more obvious that you should remove the separate check for None before calling the function.
There was a problem hiding this comment.
I tried to implement this with ef78d76, where I moved _max_order_circular_harmonics to util, renamed it to max_order_circular_harmonics and added documentation.
For the documentation I would need some help. At the moment I added page 132 of [Ahrens2012]_, but it would be nicer to replace this by the actual equation. As I don't have a copy of the book here I cannot do this myself.
There was a problem hiding this comment.
Do you need the equation number? It's (4.26)
| phaseshift : float | ||
| Phase shift | ||
|
|
||
| """ |
There was a problem hiding this comment.
Add a reference to the help message. In sfs-matlab we used eq. (11) from Spors, Kuscher, Ahrens (2011).
There was a problem hiding this comment.
Thank you for the review.
The references are now added with the respective equation numbers.
| phaseshift : float | ||
| Phase shift | ||
|
|
||
| """ |
There was a problem hiding this comment.
Add a reference to the help message. In sfs-matlab we used eq. (10) from Spors, Kuscher, Ahrens (2011).
| for channel, cdelay in enumerate(delays_samples): | ||
| out[cdelay:cdelay + len(signal), channel] = signal | ||
| return out, offset_samples / fs | ||
|
|
There was a problem hiding this comment.
This will break the current implementation of the time based WFS driving functions.
I think we wanted to try out using the signal class, here is an example : https://github.com/sfstoolbox/sfs-python/blob/master/sfs/util.py#L141
There was a problem hiding this comment.
Thank you. Probably I chose the wrong script while merging.
Those are not all in your changes, I'm just trying to say that you should use some kind of a linter, ideally as an editor plugin or something. |
Will there ever be some orders missing? If not, I think a simple But if there is ever an order missing (or not starting from 0), this wouldn't work. Then a |
|
@mgeier Thank you, it works very well. Just simple. |
|
sfs/time/drivingfunction.py
Outdated
| s0 = (c/r)*p | ||
| sinf = (c/r0)*p | ||
| z0 = np.exp(s0/fs) | ||
| zinf = np.exp(sinf/fs) |
There was a problem hiding this comment.
Please add the used equations to the docstring.
If it makes sense, you can also move this to a separate function (and document the equations there).
|
Thanks for the updates! I've marked all my outdated requests as "resolved" and added a few more comments. Apart from this, is this PR ready for merging? If yes, can you please squash all commits and rebase to |
|
For nfchoa_25d_point() and all other related time domain NFC-HOA (and maybe somewhere else?) |
|
Variable names The gain normalization is now performed inside |
|
I agree that Also, shouldn't it be plural?
That's great! Now you can get rid of the ugly strings and just pass functions around. This, in turn, makes the appendix The signature could then look like this: def nfchoa_25d_point(x0, r0, xs, fs, max_order=None, c=None, s2z=matchedz_zpk): |
I also prefer using underscore,
which in my opinion reads better and is more self explanatory.
Great idea, thanks! |
Hmm ... This doesn't work because |
Well at some point we'll have to stop making the names longer ... This would have to be If you prefer it, that's fine for me.
That's a risk that I would be willing to take.
Exactly.
Do you mean where it is called or where it is defined? The default argument must be available where the function is defined. That means that you'll have to move the definition of OTOH, |
We are already using variable/function names like
Although passing the functions is very a fancy idea, From def nfchoa_25d_point(x0, r0, xs, fs, max_order=None, c=None, s2z=matchedz_zpk):I would expect that nfchoa_25d_point(x0, r0, xs, fs, max_order, c):and nfchoa_25d_point(x0, r0, xs, fs, max_order, c, s2z=matchedz_zpk)are the same, but actually the second one will work only if from sfs.util import matchedz_zpk
d = sfs.time.drivingfunction.nfchoa_25d_point(
x0, r0, xs, fs, max_order, c, s2z=matchedz_zpk)or d = sfs.time.drivingfunction.nfchoa_25d_point(
x0, r0, xs, fs, max_order, c, s2z=sfs.time.drivingfunction.matched_zpk) |
It is inconsistent in the sense that the first two words are directly joined together, while the second and third word are joined by an underscore. I think we should choose one or the other. But PEP-8 doesn't say anything about this: https://www.python.org/dev/peps/pep-0008/#function-and-variable-names If you really prefer
Is it also confusing that This is just how Python works: names have to be defined. And function definition may happen in a different namespace than function use. Users will have to know a few Python features in order to use the SFS toolbox. And if they don't know those features, we can give them an opportunity to learn, which is great! |
|
3D NFC-HOA driving functions for virtual plane waves and point sources are added.
The source signals are band-limited impulses (22 kHz and 1 kHz). |
|
Equations describing the NFC-HOA driving functions are added in the docstring (c600808). @mgeier |
Yes you can and you should! You can use inline math with .. math::
xJust don't forget to use a raw string |
Great! Thank you. |
Sorry, I missed that. They're changed now. |
2180e60 to
4395c15
Compare
I really appreciate your review and comments! Merging from the
The code are updated according to the recent changes. But when I build the HTML pages, the math symbols like |
|
Thanks a lot @narahahn, I think it's time to merge this! The documentation for the new return values is missing, but we can add this at a later time.
Well some people like this workflow and they keep all those merge commits in their history. I don't like this, because I think it makes the history basically useless. But anyway, now it's done, which is great!
I think this is a problem with your local files, I don't see this problem. You should probably remove the old "build" directory and try again. |
NFC-HOA driving functions are implemented using first/second-order section filters.
To be discussed
scipy.signal.besselap. But I'm not so convinced whether this is really necessary, considering that the cut-off frequency for 150th order is already around 20 kHz.