search

sfstoolbox / sfs-matlab

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

New interpolation handling #130

Closed VeraE closed 7 years ago

VeraE commented 7 years ago

@trettberg and me continued the work on enabling interpolation in 2D and propose this as the new way interpolation is handled. This is related to #76.

It is now possible to interpolate HRTFs and BRIRs between three points on sphere. The three points and their weights are selected by Delaunay triangulation by the function findconvexcone(). The function is also able to interpolate between points located on a spherical cap or on a cirlce/arc (using two points only then). Test cases are demonstrated in the validation script test_interpolation_point_selection(). The function findnearestneighbour() has been modifed to return weights and indices in the same way as findconvexcone(). This enables a concise implementation of the interpolation procedure itself. Only the function interpolate_ir() is used, interpolation() is unnecessary now. A new config struct parameter conf.ir.interpolationpointselection has been added to choose between the point selection methods.

We are thinking about changing the name of findconvexcone() since it is not perfectly suitable anymore. Maybe findsurroundingsimplex() or findenclosingsimplex()? Any suggestions are welcome.

What do you think of these changes for interpolation handling?

hagenw commented 7 years ago

Cool, I will start adding a few comments to the single changes.

fietew commented 7 years ago

I would propose to use .' or transpose() instead of ' to transpose matrices. 'yields in the conjugate transposed matrix, which for real values is no big deal, but using .' or transpose() consistently could prevent bugs.

VeraE commented 7 years ago

Is the use of .' or ' already consistently used in the whole SFS toolbox? Because there will be no problem in the new code with it as it is at the moment.

hagenw commented 7 years ago

I'm still not so comfortable with the new version of get_ir() there are lots of cart2sph() and sph2cart() entries which looks not really elegant. In addition the code for the part on % Get nearest neighbour point or points for interpolation is repeated for SimpleFreeFieldHRIR and MultiSpeakerBRIR. The last point can be solved by creating an additional function. For the first one, I'm not sure what might be the easiest solution. For example, the part starting at line 146 that tries to incorporate the head orientation, is this only possible in spherical coordinates or can it maybe directly be included in line 142?

fietew commented 7 years ago

@VeraE: Neither is, I think. Maybe this pull request is a good opportunity to agree on a consistent use of .' for this and future pull requests. Off course, revising the whole existing code is not worth the effort, imho.

fietew commented 7 years ago

I'm still not so comfortable with the new version of get_ir() there are lots of cart2sph() and sph2cart() entries which looks not really elegant.

I think, the default conversion strategy starting in https://github.com/sfstoolbox/sfs-matlab/blob/a7ac6bf764de0136782880d765e37dc1a86604ef/SFS_ir/get_ir.m#L116 (from spherical to cartesian coordinates) might not be optimal. One could save many conversions, if the coordinates are converted to spherical coordinates by default.

VeraE commented 7 years ago

  1. I can create a new function for the repeated part of the point selection. I think I better do this after we agreed on the structure of the rest of the function.

  2. I really thought a while about the conversions from spherical to cartesian coordinates and back. I don't like it that much either but I could not come up with a better way that is at the same time easily to understand if you go through the code line by line. This too applies to the example of the incorporation of head orientation starting at line 146. Are you concerned about it because it could be more efficient or because you think it's confusing? And I don't think that it is better to start with spherical coordinates in get_ir() as cartesian coordinates are needed in the beginning at line 142. @fietew Or maybe you have got an easy solution to combine lines 142-150 in one step in spherical coordinates? That would be great!

  3. I'll go through the code in this pull request and make sure ' and .' are used consistently.

hagenw commented 7 years ago

  1. @VeraE I think you can start already writing the function as I don't think we will change the whole structure of get_ir(). Maybe it is a good idea to write the function as a subfunction at the end of get_ir()?

  2. My main concern with all these coordinate system switches is that we have a very high numbers of line numbers and the code is becoming harder to understand. The performance I have not tested yet, maybe we should also check this. I was never good at coordinate system transformation, and I'm also wondering if line 142-150 could be done in one system.

VeraE commented 7 years ago

  1. I just moved the point selection part to a subfunction.

  2. Maybe there exists a nice transformation to avoid some coordinate system switches, but same as you, I am not very good at that which is of course one reason why I had to write it this way. The other reason is that I prefer some more simple lines of code over a short elegant version that needs additional knowledge to understand as long as it is not a matter of efficiency and I do not consider the coordinate system switches as relevant to efficiency. I tested the calculation of an SSR ready data set (161 head angles in horizontal plane, WFS array with 16 loudspeakers) with nearest neighbours interpolation in this here new version against the master branch. The new version was faster by about 30% which is quite nice I think.

hagenw commented 7 years ago

  1. Cool, looks good.

  2. For me it would be fine to merge the current version and create an issue reminding us, that there is room for improvements regarding coordinate transformations.

hagenw commented 7 years ago

I have another comment regarding the return value x0_new. It might be a little bit confusing that we return only the loudspeaker position and not the head orientation. And for SimpleFreeFieldHRIR we are mixing loudspeaker position and head orientation, but of course we are not doing it for MultiSpeakerBRIR.

I have further checked all other functions of the Toolbox and found no single usage of the return value x0_new. So maybe we can make get_ir() a little bit easier by just removing that parameter altogether. @VeraE what do you think?

VeraE commented 7 years ago

Creating an issue as reminder for improvement of the coordinate transforms is fine with me, too.

Regarding x0_new: I considered this only as an informative value. It was there before so I kept it the same way. I just had to change the way it was calculated. It's fine with me to delete it, though. This would indeed make the function clearer to read. Do you just delete it?

hagenw commented 7 years ago

Yes, I will do it.