How to organize loading / writing of high-level features? - Githubissues

CPJKU / madmom

Python audio and music signal processing library

https://madmom.readthedocs.io

Other

1.33k stars 204 forks source link

How to organize loading / writing of high-level features? #302

Closed fdlm closed 6 years ago

fdlm commented 7 years ago

Currently, loading and writing functions for features such as tempo, beats, notes, etc.. are spread across different files.

For beats, we have evaluation.beats.load_beats, and use utils.write_events to write beats.
For onsets, it works similarly.
For notes, we have features.notes.load_notes (which is not used anywhere as far as I can tell), features.notes.write_notes to write notes, and evaluation.notes.load_notes to load notes.
For chords, we have features.chords.load_chords and features.chords.write_chords.

I find this a bit confusing. Either we create a utils.io package that contains all loading and writing functions, or we specifically say that all loading functions have to be in the respective evaluation package, while writing functions should be in the corresponding features package. I think the former would give us a nicer package structure.

superbock commented 7 years ago

Yes, this is definitely a bit cluttered and could benefit from a cleanup. First of all, I have no strong opinions towards one of the variants or the other. I think collecting them at a single place is definitely a good thing, whereas defining it in place where it is actually needed has also a valid point. Having this said, I also lean towards an io (maybe even without utils) package variant.

fdlm commented 7 years ago

If we have a global io package, should we also put the audio file loading stuff (ffmpeg, ...) there?

superbock commented 7 years ago

That's a good question. It would make everything more clear/structured I guess. We could also have sub-packages like io.midi, io.audio, etc. then.

Some functionality of audio.signal (LoadAudioFileError, load_wave_file, write_wave_file, load_audio_file are good candidates, however I am not too sure about Stream class, I think this still belongs in audio.signal...) and all of audio.ffmpeg could be moved to io.audio and simply be imported from there, so most functionality would still be available as it is now.

superbock commented 7 years ago

One additional comment regarding MIDI: we should probably switch to mido, especially since it would be nice to have real streaming MIDI-out (and not only output to MIDI files).

superbock commented 7 years ago

@fdlm are you working on this? If not, I can draft something.

fdlm commented 7 years ago

Not right now. I wanted to finish #309 (chord eval) first, because it is also affected (loading functions all over the place), and tidy up afterwards. But if you want, then go ahead :smile:

superbock commented 7 years ago

I started working on this, but unfortunately many writer functions have some additional helper functions as well. Moving them all to a central io package makes this rather cluttered as well 😞. Adding sub-packages like events, beats, notes etc. might clean up things a bit, but then they are rather difficult to access again.

Maybe the better solution is to have all the writing functions in the respective features and loading functions in the respective evaluation package and just import them into the io package so they can be accessed centrally (of course this can be also done if they are put into io.* packages).

fdlm commented 7 years ago

Hm, isn't it just for write_notes? (see PR #312)

superbock commented 7 years ago

That's true. Actually it seemed to look worse yesterday in my draft :man_shrugging:.