removed intermediate vec and indices

[wagnerf42]

hi, i've been taking a look at your nice crate.

I did a few changes you might want back:

I switched to a more functional style of programming:

• avoiding indices when possible (this should increase perfs)
• avoiding collect into vec when possible (again, maybe increase perf)
• avoid match on options and use map instead (just for a better code
  IMO)

overall it does get 5% faster on my machine on a dumb benchmark

[alesgenova]

Hey thank you so much for the contribution, at first glance it looks really good, I just want to get around to testing it with the demo app (https://github.com/alesgenova/pitch-detection-app) to make sure everything still works as intended.

As you probably could tell from reading the code, I'm not really proficient in rust, (this was a little project to teach myself), so it's great to discover some better patterns than the ones is used (iterators, map, etc.)

Again, tank you for taking the time to write this!

[alesgenova]

Btw, I forgot to ask, what did you use for the automatic code formatting?

[wagnerf42]

Hey thank you so much for the contribution, at first glance it looks really good, I just want to get around to testing it with the demo app (https://github.com/alesgenova/pitch-detection-app) to make sure everything still works as intended.

yes for sure, it seems fine with the tests i did but it's always difficult to guarantee there is not a mistake somewhere.

As you probably could tell from reading the code, I'm not really proficient in rust, (this was a little project to teach myself), so it's great to discover some better patterns than the ones is used (iterators, map, etc.)

Again, tank you for taking the time to write this!

thank you for writing your code in the first place, it's a big help for me.
I hope to do more contributions.

[wagnerf42]

Btw, I forgot to ask, what did you use for the automatic code formatting?

rustfmt is auto-formatting the code. it is absolutely a must-have for rust since it can be easily interfaced with editors and it splits very long lines.

[wagnerf42]

so, i've been looking a bit more at the code.
if i understand correctly detect_peaks is looking at largest subranges of arr with positive clarity and returning the max of each range. this could be done very clearly using Itertool's group_by.
we could group by the sign of clarity, discard all negative ones and take the max on each positive group.
this would add a dependency to itertools though. if you'd like I can do it.

[alesgenova]

So I finally got around to testing your changes in the demo app, and it all works well!
I'm going to merge this PR right now.

Regarding the improvements you're proposing for the detect_peaks function, they sound good to me.

I just want to make sure that the new dependency doesn't break the compatibility with compiling to wasm or increase the binary size significantly.

Btw, I'm curious, are you planning on using the library in any of your projects?

[wagnerf42]

So I finally got around to testing your changes in the demo app, and it all works well!
I'm going to merge this PR right now.

that's nice. thanks, i'll work on the next one then :-)

Regarding the improvements you're proposing for the detect_peaks function, they sound good to me.

I just want to make sure that the new dependency doesn't break the compatibility with compiling to wasm or increase the binary size significantly.

yeah that's what i thought. i'll definitely take a look at the size.

Btw, I'm curious, are you planning on using the library in any of your projects?

i have a sdl player for abc files which listens to you and color notes green or red according to what you play.
i need to train more on irish reels at high speed.
i'm going to publish it as soon as it is production ready.
it's actually almost good now. i have just one remaining problem:
sometimes on my mandolin when I struck an empty string and move on to the next note on another string then the empty string keeps resonating. I thought I would have been able to see both peaks in the detect_peaks output but only one frequency is here. each note individually is detected fine but both together and only one frequency is detected.

i'm no audio guy. did you base yourself on this paper : http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.380.2575 ? or on mcleod's phd thesis ?

i'm quite happy with your crate but
i'm also going to try https://crates.io/crates/aubio-rs which has some pitch detection algorithms to see if it can solve my remaining pb.

[alesgenova]

i have just one remaining problem:
sometimes on my mandolin when I struck an empty string and move on to the next note on another string then the empty string keeps resonating. I thought I would have been able to see both peaks in the detect_peaks output but only one frequency is here. each note individually is detected fine but both together and only one frequency is detected.

I'm not an expert on the subject, but it is my understanding that most pitch detection algorithms (autocorrelation, mcleod, zero crossing,..) are really only designed for monophonic samples (i.e. single notes). For something like chords a different (and probably more complex?) class of algorithms should be used, but I don't know anything about it

i'm no audio guy. did you base yourself on this paper : http://citeseerx.ist.psu.edu/viewdoc/summary?doi=10.1.1.380.2575 ? or on mcleod's phd thesis ?

Yes, that's the paper I used as a reference for the implementation, I didn't look at the thesis.

[alesgenova]

@wagnerf42

I took a shot at detecting multiple notes using the detectors already implemented. It was just a fun attempt, but I don't have high hopes that it will work reliably.

If you want to test it out, see this PR: #6