I almost wonder if the dictionary data structure should just be used directly for storing values, if the extra return data remains part of the API.

Do you suggest using, for example, infodict['peaks'] instead of peaks? I think that would be possible. In that case infodict would always be used internally but only returned if full_output is True. However I think the trade off would be a slight decrease in readability because the line length would grow significantly...

Note: I have copied this style from scipy.integrate.odeint.

Yeah, something like that. I just wanted to make sure the point was brought up & trade-offs at least briefly considered. It looks like there was already quite some discussion of API matters in the original issue you linked.

Is the initialization of keep needed here? I'm just thinking that the first comparison below would automatically generate a bool or mask array anyway.

Yes it is needed. Look at the possible valid inputs for the argument height:

1. height=hmin -> hmax is None, only first comparison is evaluated
2. height=(None, hmax) -> hmin is None, only second comparison is evaluated
3. height=(hmin, hmax) -> both comparisons are evaluated
4. height=(None, None) -> no comparison is evaluated (has no effect but is valid)

Case 3 dictates that the second evaluation must use &= (keep must be initialized). Considering the 2nd case one can easily see that initialization is not guarantied to happen in the first comparison.

Alright, looks like a lot of input permutation handling is indeed needed.

Since this logic is repeated, I wonder if there's a way to streamline things a bit? Not sure if it is worth abstracting to i.e., a function or something or if this can be condensed to do both checks at once (maybe not because of the None pre-check requirement).

I find this function (find_peaks_cwt) particularly confusing. It mixes the signal pre-processing and the find peaks function.

Maybe it is worth including some kind of note mentioning that the (new) function find_peaks provide an easier, matlab-like, interface. This would probably make the life of someone looking for it easier.

In the long term, I think we should even considering deprecating this function.

I agree with these issues to some extend. The note could be a good idea.

But I don't like the idea of removing the function find_peaks_cwt entirely. Wavelet transformation can be a really powerful tool for noisy signals and fills a niche that the new find_peaks can't fill the same way. Improving the API and / or documentation of find_peaks_cwt could allevate the the issues you mentioned above.

Typo : length

sentence is not complete I guess, "for the lowest contour line two a symmetric interval around [...]" ?

Ah, that's a typo. The sentence should be "[...] for the lowest contour line to a symmetric interval around the evaluated peak."

When is it large? Why don't we always use it then ? and so on. I get what you are saying but it's not helping a standard user.

This is vague on purpose because I can't exactly pinpoint an array size after which there is a significant speedup. Maybe I could try to reword or expand on this in the notes? Something like:

"Searching for the peak's bases can be slow for large vector because the full vector needs to be evaluated for each peak. This evaluation area can be limited with the parameter wlen which restricts the algorithm to a window around the current peak and can shorten the calculation time significantly if wlen is significantly smaller than vector. However this may stop the algorithm from finding the true global contour line if the peak's bases are outside this window. Instead a higher contour line is found within the restricted window leading to a smaller calculated prominence. In practice this is only relevant for the largest set of peaks in vector. This behavior may even be used intentionally to calculate "local" prominences.

+1 for saying see Notes somewhere here and adding this note below, I agree it depends on a lot of factors so we can't be specific

yes see notes would be indeed better

3 more letters and we have full readability return_bases. Otherwise compute_bases feels like more scipy-ish. Also note that you are already allocating these arrays so there is nothing gained from not returning them. Had you not even allocated or touched any of those arrays then maybe for very large problems it would have made a change.

Now it's just a variable number of outputs ala matlab which drove some of us crazy. Instead you might want to just always return these and remove the bool.

Okay, I can see why this is a code smell. When thinking about it the only disadvantage / annoyance I can think of is in when a function returns many values and the user is only interested in one. In Python 2.7 the syntax

widths, *_ = peak_widths(...)

seems to be unsupported which would force the "ugly" peak_widths()[0] (maybe that's only me). We would definitely have to write the examples this way (at least as long as Scipy supports 2.7).

Anyway, I think I will remove the ret_... handles and just use a constant output size.

For peak_finding it doesn't matter anyway. Instead the current properties argument could force the function to calculate all possible properties even if no filter options are supplied. However this would convolute the if-logic a bit.

FWIW git grep ")\[0\]" | wc -l on the SciPy codebase gives 399 results (and another codebase I work on with ~200k lines gives 639 results), so I think it's a sufficiently common pattern to expect users to do it :)

Alright, that is a convincing argument. :D

after ndarray a newline is needed for rendering the type correctly.

Which type? I got this from a the example in scipy.integrate.odeint (returned parameter infodict).

I think the odeint example is less than ideal in this respect

from scipy.signal import find_peaks, peak_prominences to make it a bit more concise and to remove the signal repetition

If not used you can combine this line to the group below.

I don't quite understand. It is used in the call to vlines below. This is an extra line with comment to make the example as simple and explicit as possible.

I meant if this is only going to be used in the plotting section below you don't need to make it as a separate code block. Now it is divided into three code blocks for no apparent reason. You can see the output here

https://5174-1460385-gh.circle-artifacts.com/0/home/ubuntu/scipy/doc/build/html-scipyorg/generated/scipy.signal.peak_prominences.html#scipy.signal.peak_prominences

You actually write what I meant in the scipy.signal.find_peaks docstring 😃

what is vector?

The array to be searched for peaks. Defined in line 306 (second row) in the folded section above.

can't you just use array instead of vector which is more of a physical concept that also needs directions so on?

I used vector because I want to convey that the input is expected to be one-dimensional and find_peaks_cwt already uses this name. array is so broad we could go with x as well which seems to be the SciPy standard anyway. ts for timeseries would be possible as well but I'm not a huge fan. I think if vector is not wanted we should go with x.

Actually I would prefer the name signal but that one is even worse considering the module name... I don't suppose we could change the package name to scipy.dsp? 😉

+1 for x as we use that most other places

if wlen is not None and wlen < 4 and also don't you need to check the integerness of this?

Not really. To calculate the window borders I do this:

wleft = peak - int(wlen / 2)
wright = peak + int(wlen / 2)
# Handle border cases
if wleft < 0:
    wleft = 0
elif vector.size < wright:
    wright = vector.size
# Use slice for prominence calculation
window = vector[wleft:wright]
# Correct peak position in vector
peak -= wleft

So wlen / 2 get's floored / converted to an integer anyway.

And 3 is a valid window range although not really useful.

Ah OK, then my first comment is only about using negated ifs. wlen < 4 is easier to read.

Also in your second code snipped, if the first branch of if is done wleft<0 then it won't go into the second check right?

you can simply write

n = vector.size
window = vector[0 if wleft<0 else wleft:n if n<wright else wright]

one liner ifs are a bit dirty but if you want you can also take them out and write

n = vector.size
wleft = 0 if wleft<0 else wleft
wright = n if n<wright else wright
window = vector[wleft:wright]

and so on. I don't mean to be a python grammar-nazi but just suggesting a bit more cleaner flow of code. So that someone else can follow the logic before installing breakpoints and other printing stuff for debugging when needed.

I'm happy to learn so feel free to be a Python grammar-nazi. :) I'm happy to accept your second suggestion. But why is wlen < 4 easier to read than wlen < 3?

I think that bit is just a typo, his suggestion is essentially the same as mine regarding the wlen check -- don't negate with not, instead change the relational operator itself without changing the effective truth table

Why a permissions change on this file?

What do you mean? I suppose you are not speaking of the new line wrapping?

No I'm not (though I wouldn't change that either, other modules I see don't line break like this), look at the diff of this file on GitHub you'll see the permissions were changed 100644 → 100755

Ah, I see what you mean. That was unintentional. I must have messed that up somehow. I'll revert that as well as the line wrapping.

Some of the builds / test fail with the the warning:

FutureWarning: Conversion of the second argument of issubdtype from int to np.signedinteger is deprecated. In future, it will be treated as np.int64 == np.dtype(int).type.

I have a feeling that using np.issubdtype(peaks.dtype, np.integer) would be the fix for this but I can't reproduce this warning on my system. Any insights before I blindly push this?

not 3 <= wlen is not so readable, I'd change this to:

if wlen is not None:
    wlen = operators.index(wlen)  # ensure int-ness
    if wlen < 3:
        raise ValueError(...)

... then here you can do peak + wlen // 2 instead of this int conversion

Is there a more authoritative reference for this (journal article)?

Also, typo in Topographic on the line above

I don't have a satisfying response as I come up empty handed when searching for papers. The stuff that I do find is in line with the linked article and deals with topographic analysis of peaks and mountains (nothing DSP related). Of course I could use some of the links on that Wikipedia article itself but I think that wouldn't improve the situation.

Honestly I first used Matlab's explanation as a guideline and later this Wikipedia article. Maybe something can be found later on but for now I'd like to leave this reference as it explains the term "prominence" (although 2D) in more detail than appropriate for this docstring.

Of course I could use some of the links on that Wikipedia article itself but I think that wouldn't improve the situation.

Yeah those don't look useful.

I had a look as well, it just doesn't seem to be a thing in DSP. So let's leave the Wikipedia reference

We don't use this section anymore, content should all go under Parameters

It would be good to have one or two sentences here about what this function's limitations are - e.g. for noisy signals the approach doesn't work too well, for that it's better to use find_peaks_cwt.

Something like:

"Because these filters depend on the calculation of properties for each peak this function can be slow for noisy data that has many small peaks relative to its length. In this case it is recommended to smooth the signal before peak finding and / or reduce the number of peaks with fast filter options first."

This feels more like it belongs in the Notes section.

I wasn't thinking about speed, more about accuracy. Something like "This function directly searches for maxima, hence for noisy signals the determined peak locations can be off if the noise changes the position of a local maximum. In those cases, find_peaks_cwt or a peak fitting method may be more appropriate to use".

And yes, you're probably right that this should go in the notes section.

Yeah, I should have thought of that. I definitely add this or something like it either here or in the Notes section. 👍