search

alesgenova / pitch-detection

A collection of algorithms to determine the pitch of a sound sample.
MIT License
231 stars 27 forks source link

Add docs to pitch detectors #17

Closed siefkenj closed 3 years ago

siefkenj commented 3 years ago

This PR adds basic documentation for the pitch detectors.

It needs a rust version >= 1.48 to compile the docs correctly. I used tips from rust-latex-doc-minimal-example to allow the rendering of LaTeX formulas in the docs. Unfortunately, because of the configuration, only the docs of pitch-detection can be built...That is, you must use the command

cargo doc --no-deps

to successfully build the docs.

I had some questions/comments when going through and documenting things.

  1. I omitted internals from the docs. But, it is still exported as pub. Should this be the case? It would be good if people don't rely on internals...
  2. I separately exported Pitch from internals, since I hid internals.
  3. Should utils be exported publicly?
  4. Every pitch detector uses the same interface to its ::new() function. Should this be moved to the PitchDetector trait? I don't know the convention in Rust about having ::new() on a general trait vs. just doing it as a struct-specific trait each time...
alesgenova commented 3 years ago

Hey Jason,

Thank you for yet another contribution! :) Just wanted to let you know I'm in the middle of a big move and wasn't able to look at it yet. Give me a few more days to settle in and I'll review/merge it. After that it would probably a good opportunity to tag a new release to Cargo, since we have the new detector and the improved docs

siefkenj commented 3 years ago

Good luck on the move!

I think another nice-to-have for the next release would be a query interface so Javascript code could get a list of all available algorithms.

I'm currently working on setting up some benchmarks (similar to the ones done in the original YIN/etc. papers), and I've gotten a bit distracted updating plotters to make prettier charts...

siefkenj commented 3 years ago

@alesgenova Any update on this?