sfstoolbox / sfs-matlab

SFS Toolbox for Matlab/Octave
https://sfs-matlab.readthedocs.io
MIT License
96 stars 39 forks source link

3D time-domain NFCHOA driving function #149

Closed fietew closed 7 years ago

fietew commented 7 years ago

Follow up to #117: Supports plane waves and point sources

use test_driving_functions_imp_with_delay to test

fietew commented 7 years ago

There seems to be a bug secondary_source_positions: The additional weighting with cos(theta) for spherical arrays is wrong imho. It leads to different results if the source is not in the xy-plane. I added two additional examples to test_driving_functions_imp_with_delay.

hagenw commented 7 years ago

You removed the additional weighting now completely. Do we not need some kind of weighting for integration on a sphere?

fietew commented 7 years ago

The integration weights are defined by the quadrature scheme, i.e. the spherical grid. The different grids should already yield the correct integration weights. The cos(theta) weighting is only correct for Gauss grids, imho. Try [~, w] = legpts(100), plot(w).

hagenw commented 7 years ago

OK, I see. We have a Gaussian grid in the toolbox, are the weights correctly given there? See https://github.com/sfstoolbox/sfs-matlab/blob/master/SFS_general/get_spherical_grid.m#L108

fietew commented 7 years ago

I tried this:

conf = SFS_config;

conf.secondary_sources.geometry = 'sphere';
conf.secondary_sources.size = 2.0;  % unit sphere

conf.secondary_sources.grid = 'gauss';
conf.secondary_sources.number = 200;
x0 = secondary_source_positions(conf);
sum(x0(:,7))/(4*pi)

conf.secondary_sources.grid = 'fabian';
conf.secondary_sources.number = 11345;
x0 = secondary_source_positions(conf);
sum(x0(:,7))/(4*pi)

conf.secondary_sources.grid = 'equally_spaced_points';
conf.secondary_sources.number = 900;
x0 = secondary_source_positions(conf);
sum(x0(:,7))/(4*pi)

The FABIAN grid has some issues. However, it is not a full sphere grid and its not a unit sphere.

hagenw commented 7 years ago

The problems with the FABIAN grid existed before as well. If I execute your code with the master I'm getting:

0.7858
96.7188
0.7854

and now

1.0000
141.2893
1.0000

So, your fix for the secondary source integrations weights seem to be correct.

Regarding the integration weights for FABIAN, I guess there where given together with the data set, but I'm not sure. In the official release of the data set the state that the weights where calculated by rectangles around the grid points.

hagenw commented 7 years ago

I tested it and it worked. In my opinion this is now ready to merge.

I'm tempted to clean up the commit history by some squashing, but as this includes a merge of the master and also some code from another pull request, the safest might be a merge?

hagenw commented 7 years ago

I created a new branch nfchoa_imp_3D_squash, where I modified the commit history a little bit. That branch I can easily rebase on master if this is fine with you.

fietew commented 7 years ago

Please do not rebase the nfchoa_imp_3D_squash branch onto master, as this would destroy the connection between the changes and this pull request on github. Please modify the commit history of nfchoa_imp_3D in the same way as you have done it for nfchoa_imp_3D_squash and then rebase the former onto master.

hagenw commented 7 years ago

That's true, thanks for the hint.