Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make peak_local_max return indices always sorted #2432

Closed
aomader opened this issue Jan 6, 2017 · 4 comments
Closed

Make peak_local_max return indices always sorted #2432

aomader opened this issue Jan 6, 2017 · 4 comments
Labels
🔰 Good first issue A good task for those new to the library ⏩ type: Enhancement Improve existing features

Comments

@aomader
Copy link
Contributor

aomader commented Jan 6, 2017

Currently, the method peak_local_max returns sorted indices (w.r.t. the intensities) if the number of indices is > num_peaks. I do understand that it is nowhere stated that peak_local_max does that, it is merely a side-effect, but I found it quite convenient.

Different behaviour arises when the number of indices <= num_peaks, in which case the indices are not sorted.

This is somewhat counter-intuitive and I therefore propose to make it the documented default to return indices sorted by their intensities in decreasing order.

I would be willing to write a PR for this.

@soupault
Copy link
Member

soupault commented Jan 6, 2017

Sounds like a good change. 👍 from me.

@soupault soupault added 🔰 Good first issue A good task for those new to the library ⏩ type: Enhancement Improve existing features labels Jan 6, 2017
@ahojnnes
Copy link
Member

ahojnnes commented Jan 8, 2017

I think, this is a useful addition and shouldn't impact perfromance too much.

@kevcolin
Copy link

kevcolin commented Jan 9, 2017

Hello guys,
I've faced some problems recently when trying to use peak_local_max. I've asked a question on stackoverflow (http://stackoverflow.com/questions/41550022/typeerror-peak-local-max-got-an-unexpected-keyword-argument-num-peaks-per-la). Maybe some of you could take a quick look please ? Considering I'm a beginner, it's very likely that the function is fine and that I did something wrong, but still. Thank you (Edit : hi again, somebody pointed out I was just reading the wrong version of the documentation, sorry for disrupting)

@soupault
Copy link
Member

Closed via #2435 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔰 Good first issue A good task for those new to the library ⏩ type: Enhancement Improve existing features
Projects
None yet
Development

No branches or pull requests

4 participants