Chord Evaluation #309

fdlm · 2017-07-11T07:02:44Z

Adds evaluation functionality for chord sequences.

TODO:

Add tests for the evaluation scores.
Change CHANGES.rst

Comments are welcome.

superbock

I can only give feedback regarding the formatting which is generally very nice. I added some comments regarding PEP 257. Also numpydoc usually ends descriptions of parameters with periods (which you sometimes use and sometimes not). I also find it nicer if there's a blank line before the closing """ of multi-line docstrings. But these are only minor details :)

superbock · 2017-07-11T07:56:28Z

madmom/evaluation/chords.py

+            (chords['intervals'] == _shorthands['7']).all(axis=1) |
+            (chords['intervals'] == _shorthands['min7']).all(axis=1) |
+            (chords['intervals'] == _shorthands['maj7']).all(axis=1) |
+            (chords['intervals'] == NO_CHORD[-1]).all(axis=1))


You could probably call select_majmin and only add the sevenths (7, min7, maj7).

superbock · 2017-07-11T08:09:15Z

madmom/evaluation/chords.py

+
+def pitch(pitch_str):
+    """
+    Converts a string representation of a pitch class (consisting of root


PEP 257 suggests to use rather "do this" than "does..." style, i.e. "convert...".

superbock · 2017-07-11T08:11:34Z

madmom/evaluation/chords.py

+    """
+    Converts a string representation of a musical interval into a semitone
+    integer (e.g. a minor seventh 'b7' into 10, because it is 10 semitones
+    above its base note).


Same as above ('convert"). Additionally PEP 257 suggests that "multi-line docstrings consist of a summary line just like a one-line docstring, followed by a blank line, followed by a more elaborate description." However, I am not sure if it could be split like this. I don't mind if not.

superbock · 2017-07-11T08:13:44Z

madmom/evaluation/chords.py

+def evaluation_pairs(det_chords, ann_chords):
+    """
+    Matches detected with annotated chords and creates paired label segments
+    for evaluation.


Same again: match... create...

superbock · 2017-07-11T08:15:13Z

madmom/evaluation/chords.py

+def segmentation(ann_starts, ann_ends, det_starts, det_ends):
+    """
+    Compute the normalized Hamming divergence between chord
+    segmentations as defined in [1]_ (Eqs. 8.37 and 8.38).


Did you check if this links to the reference when docs are created?

…ction files

…ion.

superbock · 2017-07-24T17:21:33Z

bin/evaluate

@@ -13,7 +13,7 @@ import argparse
 import warnings

 from madmom.utils import search_files, match_file
-from madmom.evaluation import onsets, beats, notes, tempo, alignment
+from madmom.evaluation import onsets, beats, notes, tempo, alignment, chords


I started re-arranging the imports alphabetically, because they are easier to read and searchable. Although not explicitly stated in PEP8, this is also recommended by various Python style guides. You could also do so when adding a final commit...

Ok. Since they were not sorted alphabetically until now, I did not care. I'm going to change that.

superbock · 2017-07-24T17:22:00Z

madmom/evaluation/__init__.py

@@ -886,12 +886,13 @@ def evaluation_io(parser, ann_suffix, det_suffix, ann_dir=None, det_dir=None):


 # finally import the submodules
-from . import onsets, beats, notes, tempo, alignment
+from . import onsets, beats, notes, tempo, alignment, chords


superbock · 2017-07-24T17:23:45Z

madmom/evaluation/chords.py

+
+"""
+
+import numpy as np


Minor detail: most import style guides suggest an empty line.

ok. ¯_(ツ)_/¯

superbock · 2017-07-24T17:25:44Z

madmom/evaluation/chords.py

+    ----------
+    intervals_str : str
+        List of intervals as comma-separated string (e.g. 'b3, 5').
+


Unnecessary empty line.

Will fix this.

superbock · 2017-07-24T17:26:35Z

madmom/evaluation/chords.py

+    ----------
+    det_chords : numpy structured array
+        Chord detections with 'start' and 'end' fields.
+


Unnecessary empty line.

Will be fixed.

superbock · 2017-07-24T17:33:58Z

madmom/evaluation/chords.py

+        ('undersegmentation', 'UnderSegmentation'),
+    ]
+
+    def __init__(self, detections, annotations, name, **kwargs):


I am not sure if a name is really required. Most other evaluation classes don't do so. If you change it to name=None, please update the docstring accordingly.

Sure, I'll change it to name='' directly...

superbock · 2017-07-24T17:37:43Z

madmom/evaluation/chords.py

+    return seg / (ann_ends[-1] - ann_starts[0])
+
+
+class ChordEvaluation(object):


I think this class can inherit from madmom.evaluation.EvaluationMixin. This mixin provides the metrics property, a dict containing all metrics defined in METRIC_NAMES.

Actually, I'm not sure what this MixIn is for:

Every evaluation implements its own to_string method.

The __len__ method is not used anywhere (in any case, it can only return an integer, so it is quite limited)

The metrics property is not used anywhere except for the AlignmentEvaluation

The formatters tocsv and totex only use METRIC_NAMES, which has to be defined for each evaluation class anyways.

So, what advantages does the Mixin give us?

Not too much, but as said previously, it automatically provides a uniform way of accessing all metrics via a property...

superbock · 2017-07-24T17:45:34Z

madmom/evaluation/chords.py

+        Detected chords of evaluation segments.
+    durations : numpy array
+        Durations of evaluation segments.
+    """


Minor thing: I usually add an empty line before the closing """ -- as you did in many other places.
I just mention it here, it misses multiple times.

Sure, fixed.

superbock · 2017-07-24T18:14:12Z

Ok, too late... maybe you can comment on my comments. Those are all things which can be incorporated in another commit, most of them bundled with other similar/related changes (e.g. reordering of imports).

fdlm · 2017-07-24T20:02:39Z

Yes, I wanted to have this off my TODO list so I merged it quickly 😄

Anyways, #315 should address your comments, except the EvaluationMixin thing, which we should reconsider IMHO.

superbock · 2017-07-24T20:17:18Z

Fully understand this 😄

superbock reviewed Jul 11, 2017

View reviewed changes

fdlm force-pushed the feature/chord_eval branch from e855a81 to f3f595b Compare July 12, 2017 06:13

fdlm mentioned this pull request Jul 12, 2017

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

Closed

fdlm force-pushed the feature/chord_eval branch from 97cf7ba to ad5dc1a Compare July 24, 2017 15:34

Filip Korzeniowski added 17 commits July 24, 2017 17:35

implemented chord parsing

6f10e98

first step towards chord eval

34c4d10

added some docstrings to chord evaluation

4694e96

implemented majmin and root

18ccbd3

added sevenths and sevenths bass metrics

3d7bccc

added segmentation values

335d6d6

faster loading, fixed adjustment of chord sequences

a9284e4

simplified handling of evaluation pairs

4ab389c

added some shorthands, added more docstrings

82f6a1d

more docstrings

e90a7ef

import chord evaluation by default

bc19905

fixed docstrings, added evaluation.chords to documentation

8b6f255

simplified selection of seventh chords. added more tests

50f3fbe

added segmentation test

dff5a40

added test for select_sevenths; added dummy chord annotation and dete…

21151b7

…ction files

fixed some bugs in chord evaluation. finished tests for chord evaluat…

8b532d4

…ion.

Added chord eval feature to CHANGES.rst, fixed path function test

ad2b899

fdlm force-pushed the feature/chord_eval branch from ad5dc1a to ad2b899 Compare July 24, 2017 15:35

fdlm changed the title ~~[WIP] Chord Evaluation~~ Chord Evaluation Jul 24, 2017

fdlm merged commit 852cfe8 into master Jul 24, 2017

fdlm deleted the feature/chord_eval branch July 24, 2017 17:37

superbock reviewed Jul 24, 2017

View reviewed changes

fdlm mentioned this pull request Jul 24, 2017

Minor cosmetic fixes to chord eval code #315

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chord Evaluation #309

Chord Evaluation #309

fdlm commented Jul 11, 2017 •

edited

Loading

superbock left a comment

superbock Jul 11, 2017

superbock Jul 11, 2017

superbock Jul 11, 2017

superbock Jul 11, 2017

superbock Jul 11, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock Jul 24, 2017

superbock Jul 24, 2017

fdlm Jul 24, 2017

superbock commented Jul 24, 2017

fdlm commented Jul 24, 2017

superbock commented Jul 24, 2017

		return seg / (ann_ends[-1] - ann_starts[0])


		class ChordEvaluation(object):

Chord Evaluation #309

Chord Evaluation #309

Conversation

fdlm commented Jul 11, 2017 • edited Loading

superbock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superbock commented Jul 24, 2017

fdlm commented Jul 24, 2017

superbock commented Jul 24, 2017

fdlm commented Jul 11, 2017 •

edited

Loading