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.

Sign up for GitHub

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

Jump to bottom

IO module #346

Merged
merged 31 commits into from
Apr 5, 2018
Merged

IO module #346

merged 31 commits into from
Apr 5, 2018

Conversation

superbock
Copy link
Collaborator

@superbock superbock commented Dec 7, 2017 •
edited
Loading

Changes proposed in this pull request

This PR moves all IO functionality into a separate module. This pull request supersedes #313.

  • a new io module is created which bundles all feature loading/writing functionality which was split among the different feature.x and evaluation.y modules,
  • madmom.io.audio constitutes the functionality of audio.ffmpeg and all the loading functions from audio.signal,
  • write functions do not return any data, only write it to file
  • evaluation class expect annotations and detections, file handling moved to evaluate script
  • writing of MIREX output formats moved to programs

When merged, this PR fixes #306, fixes #302 and fixes #192.

Remaining TODOs before this pull request can be merged

  • write_notes_midi should go into io.midi
  • check which load_chords function to use
  • unify loading of events/onsets/beats EDIT: ~will be~~ was addressed in a separate PR, see issue Unify behaviour of load functions. #338
  • refactor tests after the final structure is defined
  • move load_key to io
  • handling of MIREX output formats should maybe go into a separate io.mirex module EDIT: since this is only relevant for notes and tempo we'll leave it where it is with a distinct name it is cleaner to move MIREX output formatting directly into the programs
  • move filter_downbeats to io-module or rewrite as write_downbeats?
  • load/write tempo has lots of additional logic which probably shouldn't be there EDIT: moved to a separate issue loading/writing tempo has lots of logic included #356
  • expand_notes() was moved to utils, not sure if this is a good place

@superbock superbock force-pushed the io_module branch 6 times, most recently from a8489f2 to d27c6b6 Compare December 21, 2017 13:18
@superbock superbock force-pushed the io_module branch 2 times, most recently from a6a2cd8 to 7c445f9 Compare February 13, 2018 07:56
@superbock superbock mentioned this pull request Feb 13, 2018
Merged
@superbock superbock force-pushed the io_module branch 6 times, most recently from 04029c2 to 8288e19 Compare February 13, 2018 11:42
@superbock superbock requested a review from fdlm March 4, 2018 10:41
This was referenced Mar 4, 2018
Closed
Closed
@superbock superbock changed the title [WIP] IO module IO module Mar 4, 2018
superbock
superbock commented Apr 4, 2018
View reviewed changes
madmom/audio/signal.py Outdated
raise LoadAudioFileError(error)
warnings.warn(load_audio_file.__doc__)
from ..io.audio import load_wave_file
return load_wave_file(*args, **kwargs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Must be load_audio_file

fdlm
fdlm suggested changes Apr 5, 2018
View reviewed changes
Copy link
Contributor

@fdlm fdlm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's a huge and important one. I have some minor comments that can be addressed easily, I think. I can take care of anything chord/key related.

bin/DCChordRecognition Outdated
@@ -75,7 +77,7 @@ def main():
# save the deep chroma vectors to a file
out_processor = ActivationsProcessor(mode='w', **vars(args))
else:
# load conditional random field
# regocgnise chords with a conditional random field and output them
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recognise...

madmom/evaluation/chords.py Outdated
Chords in `CHORD_ANN_DTYPE` format

"""
# TODO: Join consecutive labels of identical chords
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this TODO is obsolete. merge() does this.

madmom/io/__init__.py Outdated
SEGMENT_DTYPE = [('start', np.float), ('end', np.float), ('label', object)]


def _open_file(fn):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is probably unnecessary. It can be replaced with open_file, I guess.

tests/test_evaluation_key.py
@@ -51,20 +63,13 @@ class TestLoadKeyFunction(unittest.TestCase):

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests should be moved to test_io.

tests/test_evaluation_onsets.py Outdated


# loading function
class TestLoadOnsetsFunction(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should also go to test_io, no?

tests/test_features_chords.py Outdated
@@ -40,30 +41,32 @@ def _compare_labels(test_case, labels, reference_labels):
test_case.assertTrue((labels['label'] == reference_labels['label']).all())


class TestLoadSegmentsFunction(unittest.TestCase):
def test_read_segments_from_file(self):
class TestLoadChordsFunction(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These things should also be moved to test_io...

Filip Korzeniowski and others added 14 commits April 5, 2018 12:42
refactored chord io functions
4751c75
restructured tests
20ca616
evaluation classes expect detections/annotations not files or file ha…
df56219 
…ndles

refactor loading of detections/annotations for evaluation into evaluation
script and add tests
adapt beats loading and evaluation
74863bc
adapt chords loading and evaluation
5dca08b
adapt key loading and evaluation
69e7399
adapt notes loading and evaluation
1b11f8f
adapt tempo loading and evaluation
66181c6
remove alignment evaluation code
deb7b93
replace MIDI code in utils.midi with new io.midi module based on mido
63526d2 
utils.midi is kept and marked deprectated to enable a smooth transition
refactor writing of notes in MIDI/MIREX format
89a4d22
add separator and format arguments to all writing functions
f4ce901
refactor writing of tempo in MIREX format
fa842ce
write functions do not return any data
c4456ca
@superbock superbock force-pushed the io_module branch 2 times, most recently from fcc1825 to ddfe586 Compare April 5, 2018 11:04
fdlm
fdlm approved these changes Apr 5, 2018
View reviewed changes
Copy link
Contributor

@fdlm fdlm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a final remark, otherwise great!

madmom/io/__init__.py Outdated
from .midi import load_midi, write_midi
from ..utils import suppress_warnings, string_types

if sys.version_info[0] < 3:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that _open_file is gone, this can be removed as well (sorry, did not mention this in the previous comment)

@superbock superbock merged commit 9dd454c into master Apr 5, 2018
@superbock superbock deleted the io_module branch April 5, 2018 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fdlm fdlm fdlm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@superbock @fdlm