BUG: signal: fix peak widths zero division error #20721
Conversation
There was a problem hiding this comment.
thanks @Jerry-Ma, could you add a test which fails before this patch and passes afterwards?
|
If there is not updates there, we could consider shipping it still. It's pretty clear that the conditions guard from a divide by 0. |
|
Friendly ping @Jerry-Ma it would be really great if you would be able to add a regression test for this |
Sorry I was (and am) traveling and have limited time working on this. I'll try to see what I can do in the next few days. |
No problem, if it is easier to provide an example I can add the test |
|
Friendly ping @Jerry-Ma |
There was a problem hiding this comment.
Since we still don't have a reproducing example/test, I took a stab at "brute forcing" one using hypothesis:
--- a/scipy/signal/tests/test_peak_finding.py
+++ b/scipy/signal/tests/test_peak_finding.py
@@ -1,4 +1,5 @@
import copy
+import warnings
import numpy as np
import pytest
@@ -19,6 +20,11 @@ from scipy.signal.windows import gaussian
from scipy.signal._peak_finding_utils import _local_maxima_1d, PeakPropertyWarning
+from hypothesis import strategies as st
+from hypothesis import given, settings
+from hypothesis.extra import numpy as hynp
+
+
def _gen_gaussians(center_locs, sigmas, total_length):
xdata = np.arange(0, total_length).astype(float)
out_data = np.zeros(total_length, dtype=float)
@@ -444,6 +450,16 @@ class TestPeakProminences:
class TestPeakWidths:
+ @given(hynp.arrays(np.float64, (12,), elements=st.floats(0.001, 1), unique=False),
+ st.floats(min_value=0.001, max_value=1),
+ hynp.arrays(np.int64, (5,), elements=st.integers(0, 10), unique=False))
+ @settings(max_examples=10_000)
+ def test_gh_20720(self, x, rel_height, peaks):
+ with warnings.catch_warnings():
+ warnings.simplefilter("ignore", category=PeakPropertyWarning)
+ peak_widths(x, peaks, rel_height=rel_height)
+
+
def test_empty(self):but it still passes consistently even at 10_000 examples. Maybe a signal regular can tweak it to find the problem, but I'll probably move on.
|
I'll bump the milestone following my analysis above--suggestions are still welcome of course. To be honest, if we don't get a viable reproducer soon I'd be tempted to close the issue to avoid timesinks for other devs. |
I am deeply sorry for my long inactivity on this. Thanks a lot for being on top of this issue and trying to produce a test to demonstrate the problem. I also tried come up with a contrived input that could reproduce the issue, but failed. However, I was able to find in our production data that have triggered the problem. I made a dump of the data as npz file, which is attached (it has to be unzipped as I cannot upload npz file directly here), and here is the script to reproduce the error using the npz file: It is possible to isolate the exact peak that caused the issue in the dataset, but that'll take some time and investigation which I might not have the time to do. I am not sure if it is a good idea to include the entire data in the test, which is a couple hundred KB. But at least this is a starting point to properly address this issue. |
Thanks, I think we'll need a bit more time to see if we can cut this down to a reasonable minimum viable regression test. I need to focus on other aspects of the release process for the next few days, but I'll keep a milestone on this for the next release, and if we resolve it before then we can always backport it since it is a bug fix. |
Reference issue
Closes #20720
What does this implement/fix?
Added check to only do interpolation if values are different.
Additional information