sfstoolbox / sfs-matlab

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

Add voronoi interpolation to HRIR processing #171

Closed feliximmohr closed 5 years ago

feliximmohr commented 6 years ago

Initial voronoi interpolation implementation as discussed in #151.

hagenw commented 6 years ago

Cool, we are finally on the way to have a proper point selection for 2D and 3D cases.

@feliximmohr could you have a look at the test_interpolation_point_selection() test function that we have for the findconvexcone() method and see if you could integrate the new Voronoi method to compare to the old one? If you are aware of more interesting test cases for the Voronoi method, feel also free to add them there.

feliximmohr commented 6 years ago

test_interpolation_point_selection()

findvoronoi()

hagenw commented 6 years ago

Great work, I was able to run test_interpolation_point_selection() under Matlab and Octave and it looks very good.

I still have a few questions:

  1. For the 3D cases the Voronoi algorithm selects most of the time 4 points, even if it is not very intuitive (see the first plots by the test function). I guess this is this desired behaviour?
  2. The weights for the test case "2D grid, arc with gap smaller than 180 deg" are different between findconvexcone and findvoronoi. The one from findconvexcone look a little bit more plausible to me.
  3. In general we don't care for the actual weight values in the tests (use NaN instead). Should we change this and add the expected values or isn't it worth the effort?
  4. The point selection for the non-sphere case worked quite well. Is this always the case with Voronoi? Then we could disable the corresponding warning.
fietew commented 6 years ago
  1. For the 3D cases the Voronoi algorithm selects most of the time 4 points, even if it is not very intuitive (see the first plots by the test function). I guess this is this desired behaviour?

It terms of Voronoi regions, it is plausible.

  1. The weights for the test case "2D grid, arc with gap smaller than 180 deg" are different between findconvexcone and findvoronoi. The one from findconvexcone look a little bit more plausible to me.

findconvexcone is based on the euclidean distances between the two selected points: You may guess, that the intersection point of the arrow and the line (not plotted) between the selected points is quite in the middle between the two points. Hence, the weights are almost equal. findvoronoi is based upon great-circle-distances, which in 2D is just difference in azimuth: You can see, that one of the selected point is closer to xs with respect to the azimuth angle. Hence, it gets way more weight. Note, that both interpolation principles become the same, if the distance between the selected points is small.

  1. In general we don't care for the actual weight values in the tests (use NaN instead). Should we change this and add the expected values or isn't it worth the effort?

For some cases, I don't know, what weights exactly are to be expected.

  1. The point selection for the non-sphere case worked quite well. Is this always the case with Voronoi? Then we could disable the corresponding warning.

Yes

hagenw commented 6 years ago

We should add an entry for Voronoi in SFS_config.m as well, see https://github.com/sfstoolbox/sfs-matlab/blob/voronoi/SFS_config.m#L384-L393 @fietew could you do this?

Is it also desirable to switch to Voronoi as a new default as it works in 2D and 3D or should we stay with the current default?

fietew commented 6 years ago

FYI: There are still some cases, were I am not completely satisfied with the results of the algorithm, so its still work in progress.

hagenw commented 6 years ago

What is the status of this? Should we include it, but stay with the current default methods or would you like to improve this first and make it the default after merging?

fietew commented 6 years ago

At the moment. I don't have the time to push this forward. We should stay with the old version and merge this after we have covered all the special cases.

fietew commented 6 years ago

@feliximmohr will be working on this PR, again. Some thoughts how the code could be (re)structured:

the term "old" corresponds to x0 the term "new" corresponds to [x0;xs]

Detection of dimensionality (to be checked by a function, e.g. check_dimensionality)

Interpolation

Remarks on computation of Voronoi regions, if all points are in one hemisphere

hagenw commented 5 years ago

@feliximmohr: thanks for your effort. I'm wondering what the current status of this pull request is. Are there still changes you would like to make or should I start and review the current code?

feliximmohr commented 5 years ago

I just pushed the last changes i wanted to make. From my point of view, the code review can be started.

hagenw commented 5 years ago

Cool, thanks a lot. I will have a look at it during the next days, hopefully until mid of November.

hagenw commented 5 years ago

Great work, I added a few comments and checking of input args, otherwise all tests run fine and I think it is ready to merge.

The only thing missing at the moment is documentaton about the method in SFS_config in the section before conf.ir.interpolationpointselection. At the moment the its default setting is 'nearestneighbour'. Is the new method better suited as a default, or should we stick with the current method?

fietew commented 5 years ago

There were some functions, which we initially planned to (re-)use for findvoronoi and findconvexcone. However, it turned out that these are either not needed or do not exhibit the exact same behavior. I removed the according functions. As a side effect, this PR does no longer include any changes for findconvexcone.

fietew commented 5 years ago

At the moment the its default setting is 'nearestneighbour'. Is the new method better suited as a default, or should we stick with the current method?

I would stick with the current (simpler) method. The Voronoi interpolation is for sure better as it covers many cases (1D, 2D, etc.) , but quiet complex (also with regard to computation time).

I think, this is ready for merge. Squash would be fine for me.