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

Added Richard Vogl's ADT models #19

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

asantos-6
Copy link

Related Discussion - #482

Related PR - #326

This PR adds Richard Vogl's [Vogl, 2018] Automatic Drum Transcription (ADT) models to the madmom library. (Link to directly download the .zip file: ⬇️).

Other comments:

It would be extremely helpful if madmom had a built-in way of performing ADT. This is already marked as Work In Progress in another PR but seems to have been dropped meanwhile.

I have managed to make the DrumTranscriptor script work locally on the drum_transcriptor branch by updating the models folder according to this PR, and by changing the /features/drums.py file in my local madmom parent repository.

However, the branch itself seems to be broken (I suspect that's the reason why this has been dropped) and some of the tests when running python setup.py pytest fail.

I also tried to update the drum_transcriptor branch from main, resolving conflicts as I presumed correctly (mainly choosing the changes from the main branch), but some tests still failed (even without my changes added to the branch).

Questions

  • Regarding the changes to make the drum_transcriptor branch work, I'm not sure if I should make two separate PRs: this one, and then one on the parent repository with the changes to the /features/drums.py file as I have never worked with submodules before; or if I should only make one PR that encompasses all the changes, both to the submodule and the parent repository. If so, how do I do that?
  • Apart from making the DrumTranscriptor script runnable inside the drum_transcriptor branch, what can I do to make this branch mergeable onto the main branch and thus incorporate ADT into madmom? Any suggestions of what I should look for?

P.S.: This is my first time contributing to an open-source library, so there are probably some things above that don't make total sense. Please bear this in mind and I apologize for some eventual lack of knowledge and know-how regarding GitHub, PRs, and how to contribute in general.

Thank you,
André C. Santos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant