Make CC-interpolation simpler & more robust

flixha commented 2 years ago

What does this PR do?

Replaces the CC-interpolation function that can be used for lag_calc and catalog_to_dd with a function that simply resamples the CC.

Why was it initiated? Any relevant Issues?

CC-interpolation with _xcorr_interp "failed" (i.e., no improvement with interpolation) more often than maybe necessary. For the example data in the test, the new function fails to produce an improved CC with interpolation in only about 90 cases, while the previous one failed in about 150 cases.
_xcorr_interp had one specific weakness: it needs more than 2 samples with negative curvature to interpolate around the peak of the CC. If seismograms are filtered just below the Nyquist frequency (e.g., sampling rate 20 Hz and low-pass filter at 9.9 Hz) and contain plenty of high-frequency content just below the Nyquist frequency, then that criterion of more than 2 samples with negative curvature is often not fulfilled (c.f. these lines)
On a dataset where the above was quite relevant, the new function improved the dt-accuracy for about 15 % more correlations than the old function (n=8*10^7)

PR Checklist

[X] develop base branch selected?
[X] This PR is not directly related to an existing issue (which has no PR yet).
[X] All tests still pass. ~- [ ] Any new features or fixed regressions are be covered via new tests.~
[X] Any new or changed features have are fully documented.
[X] Significant changes have been added to CHANGES.md. ~- [ ] First time contributors have added your name to CONTRIBUTORS.md.~

calum-chamberlain commented 2 years ago

This looks nice! Although I think this should go through a deprecation cycle - e.g. with an option to use the resampling method rather than the current method. I think the current method is documented in a couple of spots linking to a paper so we would need to find and change those as well.

flixha commented 2 years ago

@calum-chamberlain Of course, I'll fix this so that the old methods stays as the default with a deprecation message for a while, and users can choose this method. Should have mentioned: I saw that there was a comment in the function that you may have put in because you saw that it doesn't always work optimally - any more thoughts on that that we should consider for the new function?

flixha commented 2 years ago

I made the new function optional with use_new_resamp_method=True which is passed as **kwargs. The warning message is only shown once so that the log isn't completely spammed with this message - because this function is called many times when interpolation is turned on.

flixha commented 2 years ago

Tests now failing during installation, probably related to a breaking change in setuptools 61.0.0: https://github.com/pypa/setuptools/issues/3197 .. checking this now.

flixha commented 2 years ago

Sounds good, thanks for merging!

eqcorrscan / EQcorrscan