[FIX] Bounds incorrect in _drop_peak_overlap.

[ryanhammonds]

So far, the CF of one peak had been compared to the lower bound of the next gaussian. This PR fixes this so that the upper bound of one guassian is compared to the lower bound of the next. This may require adjustment to _gauss_overlap_thresh.

[TomDonoghue]

Thanks for the fix @ryanhammonds - great catch!

Notekeeping for the conversation we had:

• the current approach compares one peaks' CF to a BW-threshold of the adjacent peak (is the peak inside the threshold of the next peak). This generally works, but the code was a little unclear on what was compared (documentation issue). It also has the quirk that it can (at least in theory) be asymmetric depending on peak order (a narrow peak next to a wide peak could be a different answer depending on which is on the left, as they are compared from left to right)
• Updating the criterion to check if BW-bounds for each peak overlap (does the BW threshold of this peak overlap with the BW threshold of the next peak) seems a little cleaner and more consistent. That is what this change does.

ToDos:

• Can you add a bit more descriptive documentation into the code, to describe what & how we are comparing here?
• Can you explore if and to what extent this changes any fitting behaviour? Using the same threshold value, this would version would be more strict (drop more peaks), I think. We want to be consistent, but not necessarily drop peaks more than we do right now. We should check if we want to change the threshold, to tune it to have about the same actual fitting behaviour as before
  • Note that I think we can infer what an approximately equivalent threshold for the new approach that would mimic the old approach, and I think this might be easiest to go with.

[ryanhammonds]

I have updated the gaussian overlap threshold and added comments to make what is happening a bit more clear. The left peak's upper bound was initially the center (gaussian) frequency, but this PR has changed this upper bound to the center frequency plus 1 standard deviation, which is compared to the right peaks center frequency minus 1 standard deviation. If the left upper bound is greater than the right lower bound, then the peak with the lower height is dropped. The new _gauss_overlap_thresh has been adjusted from 1.5 to 0.75, to account for this change. I ran simulations to make sure the new threshold led to the same fitting behavior and used these simulations to confirmed the math below.

Original conditional to drop peak
guess_freq_0 > guess_freq_1 - (guess_std_1 * _gauss_overlap_thresh)

New conditional to drop a peak:
guess_freq_0 + (guess_std_0 * _gauss_overlap_thresh) > guess_freq_1 - (guess_std_1 * _gauss_overlap_thresh)

This new conditional can be re-arranged:
_gauss_overlap_thresh * (guess_std_0 + guess_std_1) > guess_freq_1 - guess_freq_0

And since guess_std_0 was zero originally, the new to old threshold can be compared:
_gauss_overlap_thresh * (guess_std_0 + guess_std_1) = _gauss_overlap_thresh * (guess_std_1 + 0)

If guess_std_0 ~= guess_std_1, then the new threshold should be half the original:
_gauss_overlap_thresh * 2 guess_std = orig_gauss_overlap_thresh * guess_std
_gauss_overlap_thresh = _gauss_overlap_thresh / 2

If the two standard deviations are not equal, then:
_gauss_overlap_thresh =_gauss_overlap_thresh * guess_std+1 / (guess_std_0 + guess_std_1)

[ryanhammonds]

Here is a link to the simulation notebook:
https://github.com/ryanhammonds/fooof/blob/fooof_explores/notebooks/gaussian_threshold.ipynb

[TomDonoghue]

Thanks for the fix @ryanhammonds

So I also had a check through that this update doesn't substantially change the fitting, compared to before, on some example data, including the stuff we use from the website, etc. It is all almost identical (+/- 1 peak in some minor cases, across groups). So I'm pretty happy that the 0.75 threshold is ~ equivalent to before, and that if & where it differs, it should be a bit of a better approach now. Merging in!