Skip to content
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

Interpolation toa #101

Merged
merged 18 commits into from
Aug 9, 2016
Merged

Interpolation toa #101

merged 18 commits into from
Aug 9, 2016

Conversation

VeraE
Copy link
Member

@VeraE VeraE commented Jun 27, 2016

I propose a different interpolation method for HRIRs that calculates a linear interpolation in the frequency domain for magnitude and phase separately. This way, the time of arrival in the HRIRs is interpolated as it is to be expected.
At the moment I added a new conf-parameter called conf.ir.interpolationmethod that can be 'simple' (= the old interpolation method samplewise in the time domain without considering the time of arrival) or 'freqdomain'.
As the new method makes use of the FFT symmetries for real signals and interpolates only one half of the spectrum, it is only a bit slower than the old method (about 2-3% on my system). Should we still keep the old method?
I could extend the new method to 2D interpolation as well.

@fietew
Copy link
Member

fietew commented Jun 28, 2016

  1. Does the unwrap of the phase really solve all the issues with the linear interpolation of the phase, i.e. does it consider all the "wrap around" effects w.r.t 2*pi?
  2. Maybe a reference for the algorithm should be added to the comments

@fs446
Copy link
Member

fs446 commented Jun 28, 2016

maybe this is inspiring:

http://iem.kug.ac.at/projects/workspace/2011/phasenabrollung-zur-interpolation-von-aussenohruebertragungsfunktionen-auf-der-kugel.html

On 28.06.2016 09:39, Fiete Winter wrote:

  1. Does the unwrap of the phase really solve all the issues with the linear interpolation of the phase, i.e. does it consider all the "wrap around" effects w.r.t 2*pi?
  2. Maybe a reference for the algorithm should be added to the comments

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#101 (comment)

@VeraE
Copy link
Member Author

VeraE commented Jul 5, 2016

Thanks for your comments. I updated the interpolation so that phase aliasing is avoided and added references.
The 2D interpolation for our case should work the same way but I would postpone that to when #76 is finished as I think this could affect the parameters that interpolate_ir() receives and possibly make the distinction in 1D and 2D unnecessary.
Should the warning for backwards compatibility better be turned off?
If this looks alright to you, I'll update the comment header.

@hagenw
Copy link
Member

hagenw commented Jul 6, 2016

Looks good, thanks.
Could you maybe create a function test_interpolation.m in the validation folder and add some code that compares the outcome of 'simple' with 'freqdomain'. Would be cool to have a nice comparison.

Regarding the warning for compatibility: I would decide on this at the end, if we see what are the differences etc.

hagenw and others added 4 commits July 6, 2016 10:28
* master:
  DOC: update plot with loudspeaker activity for WFS
  DOC: fix missing |NFC-HOA|
  DOC: update header
  DOC: rename plot script
  DOC: remove bib file
  Update link to online doc in README
  DOC: simplify sphinx command
  DOC: split into several files
  Add test_all() validation function
  Update test scripts
  Adjust length of dummy_irs in localwfs_findhpref()
  Disable ir_correct_distance N warning for HRTF esxtrapolation
  Add README for validation scripts
  Remove warning for custom grid in plot_sound_field()
  Update comments
  Fix dos line endings
  Fix #32
  Corrected Driving Functions for 2.5D WFS and add links to online equations
@hagenw
Copy link
Member

hagenw commented Aug 1, 2016

In SFS_config.m you are referencing the file test_interpolation_methods.m which should be placed in the validation folder, but this file does not exist. Could you add one?

@VeraE
Copy link
Member Author

VeraE commented Aug 8, 2016

Sorry for the delay, I had to wait for publishing of the HRTFs with low frequency correction with a new license before I could finish the test file. I have added it now. Does this look alright to you?

@hagenw
Copy link
Member

hagenw commented Aug 9, 2016

Cool, this looks nice now and I'm going to merge it.

@hagenw hagenw merged commit 7916efa into master Aug 9, 2016
@hagenw hagenw deleted the interpolation_toa branch August 9, 2016 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants