<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">- There are still "Changes requested" which I have addressed or are<br>
outdated. I don't know how to go about removing this flag.<br></blockquote><div><br></div><div>You can ping previous reviewers to take another look. They can then remove the tag.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- I think the Travis CI build fail is unrelated to my PR (something in<br>
the stats module). Perhaps someone with more knowledge should confirm this.<br></blockquote><div><br></div><div>This unfortunately happens from time to time, especially if it's on the NumPy dev/master build.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- How to handle if invalid peaks are passed to the functions<br>
peak_prominences and peak_widths? Is it okay to describe this as<br>
undefined behavior in the documentation?<br></blockquote><div><br></div><div>Yes I think so. Best to raise a sensible error if possible (e.g., argument shapes don't make sense) but I don't think it's worth spending too much time protecting against all failure modes.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- argrelmax doesn't catch peaks that are wider than one sample. Decide<br>
how to deal with this. I have implemented an alternative here which may<br>
even be faster. But I plan to make a performance comparison to confirm<br>
this. I have a feeling this should be addressed (if wanted) in a new<br>
pull request.<br></blockquote><div><br></div><div>Another PR is fine, but we should merge that one before this find_peaks PR to avoid creating backward compatibility problems. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- Should the new functionality be covered in the tutorial for<br>
scipy.signal? I feel like that would be the best place to show more<br>
complex examples which are out of place in the docstrings itself. If so,<br>
I think a new PR would be the best place.<br></blockquote><div><br></div><div>I think it makes sense to have this as part of the `find_peaks` PR. Having good / sufficient documentation of functions is worth holding off on merging. It is a good test for API sanity, and also makes it easier for others to review at a high level.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- The decision whether find_peaks should have a postfix like<br>
"find_peaks_cwt" is still pending. If so which one?<br></blockquote><div><br></div><div>There is existing discussion of this in the PR, so if people have thoughts on this, it's worth looking there to see what ground has been covered first.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Again, thank you all in advance for taking the time to review this PR<br>
and any feedback given.<br></blockquote><div><br></div><div>Thanks for working on this functionality!</div><div><br></div><div>Cheers,</div><div>Eric</div><div><br></div></div></div></div>