search

JuliaDynamics / ChaosTools.jl

Tools for the exploration of chaos and nonlinear dynamics
https://juliadynamics.github.io/DynamicalSystemsDocs.jl/chaostools/stable/
MIT License
189 stars 36 forks source link

YIN, a fundamental frequency estimator #223

Closed KalelR closed 2 years ago

KalelR commented 3 years ago

Hey, I've finished the code that was already started in PR #93 and tested it. Included here is the whole code and the test I did, comparing plots the measured quantities. It matches perfectly with the available python test, using the audio file they provide. I commented the code as it is a plot, and am not sure how to automatize. Also not sure if I should upload the audio file, or just provide a link to it.

Datseris commented 3 years ago

@KalelR I've left some comments.

  1. Can you also incorporate yin as an option in the estimate_period function? After all, the inverse of the fundamental frequency is the period.
  2. For the tests: what you should do is convert the wave file into a .csv. Then, upload this here via a pull request: https://github.com/JuliaDynamics/JuliaDynamics/tree/master/timeseries It is a folder that we upload data necessary to test things so that we do not burden the code repo.
Datseris commented 3 years ago

@KalelR I've added the hactoberfest accepted here. I've given some direction for the future, but no stress to do this before october is over.

KalelR commented 2 years ago

Reading the paper more carefully, I started to have some problems with the Python implementation that I had based the code on. So I rewrote a lot of the code, comparing closely with another Python implementation, done in the Librosa library, which makes a lot more sense. I also compared the code to several others I found on the web, including the Matlab implementation from the authors of YIN. There are some minor differences between codes, which I don't think interfere with the end result too much.

I've done some tests comparing the result from this code with the result from Librosa, and they all fit very well. Two example tests are done over chirps, which are artificially generated signals from 440 to 880 Hz. The results are:

librosa-chaostools-exponentialchirp librosa-chaostools-linearchirp

KalelR commented 2 years ago
  1. For the tests: what you should do is convert the wave file into a .csv. Then, upload this here via a pull request: https://github.com/JuliaDynamics/JuliaDynamics/tree/master/timeseries It is a folder that we upload data necessary to test things so that we do not burden the code repo.

This file's size is 2MB. It could actually be generated using SignalAnalysis.jl instead. What would be better?

Also, how would I include it in the test?

Datseris commented 2 years ago

Also, how would I include it in the test?

You download it with Base.download!

It could actually be generated using SignalAnalysis.jl instead. What would be better?

This would increase the test time more than downloading I think...? Depends on how much time install+precompile+data_production takes with this package.

KalelR commented 2 years ago

Also, how would I include it in the test?

You download it with Base.download!

It could actually be generated using SignalAnalysis.jl instead. What would be better?

This would increase the test time more than downloading I think...? Depends on how much time install+precompile+data_production takes with this package.

Ah ok, I'll update the test with this when the file is merged into JuliaDynamics :)

KalelR commented 2 years ago

I think the test is failing because DelimitedFiles is not being found. Should I add it to this package?

Datseris commented 2 years ago

tests fail.

EDIT: yes, add it. It is part of Standard Lib so it doesn't matter.

Datseris commented 2 years ago

alright we need a minor version bump in project.toml. Can you open a PR in DynamicalSystems.jl that adds this to the docs as well? So that I we don't forge to docmeunt that this exists. Just adding yin to the same @docs block that has estimate_period is enough.

KalelR commented 2 years ago

Oh, I forgot to branch on DynamicalSystems.jl and pushed directly to master. I'm sorry (didn't even know I could do that! :s). Should I redo it? Sorry again

Datseris commented 2 years ago

Unfortunately I do not know how to limit access to only non-master branches (or even if it is possible), so yes you can do that. If you have push access you have it for all branches. Nothing we can do about this now other than verting the change and force pushing to master again. But it's okay I saw the commit and is what I'd merge anyways. Just be careful next time for something more substantial.

KalelR commented 2 years ago

Just be careful next time for something more substantial.

I will, no more pushing again right before lunch!

Datseris commented 2 years ago

I'm a different person just before lunch, so I can understand.