-
Notifications
You must be signed in to change notification settings - Fork 39
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
More stable algorithm to calculate zeros of spherical hankel function #152
Conversation
@hagenw: Is it okay to remove the other methods? |
The old version of the function returned If the new method works better than the old ones, I see no big deal in replacing them as all our methods we implemented so far were buggy. |
Is the code ready for me to have a look at it? It would be cool if you could add a test function for this, maybe doing something like the figures in #57 for example those two or Figure 3 in @narahahn publication |
It is ready. I will add a test function. |
I was looking to update the reference as the Campos and Calderón paper was published in 2012, see http://doi.org/10.1155/2012/873078 I'm also wondering if we should add a link to the scipy routine as you used it as a template. Otherwise, I'm really happy that this seems to be finally solved as it bothered me since the first version that included NFC-HOA. |
Each subplot shows a subset of orders. Unfortunately the colors are missing in octave. The graphs shouldn't be all blue, but the automatic coloring is a MATLAB feature, I guess
Yes, the result is as expected.
Yes, I also had a look at the journal paper, which covers a more generalised problem and is harder to understand. I would stay with arxiv version since the formulas in the
Yes, that's a good idea. |
Ok, I updated the references accordingly. For the test function, could you maybe send me a screen shot of the Matlab result, that I can try to get the same with Octave. |
SFS_general/sphbesselh_zeros.m
Outdated
% Zeros of nth-order ordinary Bessel polynomial are the same as for | ||
% exp(1/x)*K_{n+0.5}(1/x), where K_v is the modified Bessel function of second | ||
% kind. We, hence, define the target function and its first derivative as | ||
f = @(x) besselk(order+0.5, 1./x, 1); % modified Bessel function scaled by exp |
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 remove the comment % modified Bessel function scaled by exp
as it my lead a user to expect the exp()
function to be part of that line. In addition, I think the comment provided before is sufficient.
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.
It took me a while, to figure out that besselk(nu, x, 1)
is equal to besselk(nu,x)*exp(x)
. This is why I added this comment. But if it misleading, we can remove it.
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 was a little bit confused by it.
Maybe it helps if we state explicitly the equation in the first comment. Something like
% Zeros of nth-order ordinary Bessel polynomial ... are the same as for modified
% Bessel function of second kind K_v:
% exp(1/x) * K_{n+0.5}(1/x) = ...
% Hence, we define the target function and its first derivative as
Where you have to fill the ...
part. Is it also second kind and then Y
?
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.
Could you help to fill the ...
please.
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.
According to https://en.wikipedia.org/wiki/Bessel_polynomials#Definition_in_terms_of_Bessel_functions
the nth-order ordinary bessel polynomial is denoted by y_n
, which is also the spherical Neumann function in our nomenclature. I would suggest:
% Zeros of nth-order ordinary Bessel polynomial y_n are the same as for
% the exponentially scaled modified Bessel function of second kind K_v:
% \sqrt{2pi/x} * exp(1/x) * K_{n+0.5}(1/x) = y_n
% Hence, we define the target function and its first derivative as
I added a numerical test case as well for the order 1, 10, and 100 comparing to the results from the python code. This is not perfect as the outcome depends on the ordering of the zeros in the resulting vector, which can differ for different calculation methods, but I thought it is still better than nothing. |
B(n+1) = factorial(2*order-n)/(factorial(order-n)*factorial(n)*2^(order-n)); | ||
|
||
function x = newton(f,fp,x0,TOL,MAXITER) | ||
% Newton-Rapheson method for approximation of a single root |
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.
Where comes this implementation from?
When I'm looking at scipy's newton implementation it looks a lot more complicated.
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 scipy implementation does handle many special cases and has some extensions. Here, the most simple version of the Newton-Rapheson method is implemented.
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.
Now, I found also in the scipy version. Then maybe the easiest thing is to add not a link to the function header, but simply go with the current help text for newton()
.
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 can't follow you. What do you want me to do?
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.
Nothing :)
Just a confirmation that it is fine to not add a reference link to the scipy code for the Newton method.
I also did a NFC-HOA calculation in order to compare with Hahn and Spors (2017) Fig. 8: conf = SFS_config;
conf.nfchoa.order = 30;
sound_field_imp_nfchoa([-2 2],[-2 2],0,[1.5 1.5 0],'ps',1/conf.c,conf) conf.nfchoa.order = 120;
sound_field_imp_nfchoa([-2 2],[-2 2],0,[1.5 1.5 0],'ps',1/conf.c,conf) It looks very similar, but we have the different of that prominent circular contribution which is not part of the plot in the paper. If I remember correctly we had that discussion already somewhere else, hadn't we? |
I am pretty sure, that the coloring is not relevant, but I will explicitly define the colors in the script and send you a screen shot.
What are you referring to? What is the lowest order? What is included/excluded? |
The colors in the plot work now also under Octave, but the legend was still far to big. I added a commit that replaces the legend with an updated title, indicating the shown orders for every plot. |
Coming back to the computed figure and the comparison to Hahn and Spors (2017) Fig. 8. Forget what I have set about the order. But do you have an idea why we have the obvious difference between those plots (of course besides the difference in amplitude)? |
Are you using normalisation for the plot? Also the range of the colormap in Hahn and Spors (2017) Fig. 8. is [-2,2]. |
I'm using normalisation, otherwise there is no energy in the plot. But you can do the following to get a very similar energy as in Hahn and Spors (2017) Fig. 8: conf = SFS_config;
conf.nfchoa.order = 30;
conf.plot.caxis = [-0.3 0.3];
sound_field_imp_nfchoa([-2 2],[-2 2],0,[1.5 1.5 0],'ps',1/conf.c,conf) This looks similar, but still we have a more pronounced visible circle as indicated by the arrow in the figure. |
Was #56 the discussion you meant? |
Is it possible, that you have chosen a different number of loudspeakers? In the paper, 60 has been chosen. |
Ah, brilliant, that's it: conf = SFS_config;
conf.nfchoa.order = 30;
conf.secondary_sources.number = 60;
conf.plot.caxis = [-0.3 0.3];
sound_field_imp_nfchoa([-2 2],[-2 2],0,[1.5 1.5 0],'ps',1/conf.c,conf) and we will get the same result as in the paper. I would say this is then nearly ready. There is only this discussion on the Bessel polynomial left, @narahahn should give his go, and after a small commit clean up we could merge. |
I made another test and tried to improve Fig. 3.14 from my thesis and it worked quite well. Original NFC-HOA part of the figure: New NFC-HOA part of the figure as calculated with this branch: @fietew: From my side, I have no further change requests for this branch and would like to start cleaning the commit history up, if you agree. |
I agree. |
Follow up on #71:
Based on the publication of @narahahn and @spors.
This is basically a port of a scipy routine.
@narahahn: Could you try out some parametrisation and check, if it is working as expected?