Peak finding #3729
Peak finding #3729
Conversation
jl-wynen
left a comment
There was a problem hiding this comment.
I left a bunch of comments. But please start with this:
Many arguments in kwargs should be dimensionfull, e.g., height and width, and those units have to match the units of da or be converted to those. So the arguments should be handled explicitly instead of simply being forwarded.
Additionally, this function does not forward the second return value from scipy. This means that several arguments don't function properly (they would normally trigger extra return values).
| A routine that locates "peaks" in a 1D signal. | ||
| This is a wrapper around :py:funct:`scipy.signal.find_peaks. See there for a | ||
| complete description of parameters. |
There was a problem hiding this comment.
Can you add Parameters and Returns sections? Otherwise, they are not shown properly in sphinx.
Also, please describe the return value. I.e., what do the values represent and what coords exist.
src/scipp/scipy/signal/__init__.py
Outdated
|
|
||
| from scipy.signal import find_peaks | ||
|
|
||
| if da.ndim != 1 or not isinstance(da, DataArray | Variable) or da.bins is not None: # type: ignore[redundant-expr] |
There was a problem hiding this comment.
I don't think we need the isinstance check. We don't do that elsewhere and the type hint should be enough.
Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>
Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>
Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>
Co-authored-by: Jan-Lukas Wynen <jan-lukas.wynen@ess.eu>
|
I changed my mind. It's easy for the users to obtain the peak values from the indices anyway, but it's more difficult to go the other direction. |
Adding a peak finding utility for scipp/essdiffraction#179