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

Extended chords not mapped correctly? #251

Closed
fdlm opened this issue Jul 4, 2017 · 16 comments
Closed

Extended chords not mapped correctly? #251

fdlm opened this issue Jul 4, 2017 · 16 comments
Assignees

Comments

@fdlm
Copy link
Contributor

fdlm commented Jul 4, 2017

  • Does mir_eval handle extended chords correctly? It seems to ignore everything beyond the seventh:
In [5]: mir_eval.chord.encode('A:maj(9)')
Out[5]: (9, array([1, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0]), 0)

In [6]: mir_eval.chord.encode('A:maj(9)', reduce_extended_chords=True)
Out[6]: (9, array([1, 0, 0, 0, 1, 0, 0, 1, 0, 0, 0, 0]), 0)

compared to

In [8]: mir_eval.chord.encode('A:maj(2)')
Out[8]: (9, array([1, 0, 1, 0, 1, 0, 0, 1, 0, 0, 0, 0]), 0)
  • Does it make sense to treat chords such as A:maj(9) and A:maj(2) differently in the first place? Both comprise the same pitch classes, and I doubt that one can distinguish between the two in the sound mixture of e.g. a pop/rock song.
@ejhumphrey ejhumphrey self-assigned this Jul 4, 2017
@ejhumphrey
Copy link
Collaborator

That second line (In [6]) is the bug, and should produce the same as In [8]. I've confirmed locally, and I'm a little surprised that bug got so far ... but there definitely aren't any tests for that kind of behavior. 😞

I'll try to patch soon, and will tag you in the PR.

@ejhumphrey
Copy link
Collaborator

oh, and to your second question ... I don't think mir_eval should take a philosophical stance on the difference between an add2 and a 9 chord, and the flag reduce_extended_chords lets the user do whichever they need. That said, since the second returned object is a bit-vector of active pitch classes, perhaps reduce_extended_chords=True is a more appropriate default.

That decision, however, is going to need some input from at least @craffel.

@ejhumphrey
Copy link
Collaborator

also also thanks for raising the issue 😄

@bmcfee
Copy link
Collaborator

bmcfee commented Jul 4, 2017

I don't think mir_eval should take a philosophical stance on the difference between an add2 and a 9 chord,

I'll chime in and disagree here (of course 😁). I think it would be useful to extend the chord encoding vector to a double-octave representation, so that above-octave extensions can be coded unambiguously (and distinct from, say, add2). Of course, we could retain the default behavior by collapsing out the second octave, but having the double-octave intermediate form might be useful down the line.

@fdlm
Copy link
Contributor Author

fdlm commented Jul 5, 2017

I agree with setting reduce_extended_chords=True might be a better default - this is what I would expect, at least. Would this change affect the tetrads measure? It should have no effect on the simple MajMin, Sevenths, etc. measures, at least.

Regarding the philosophical question - are there any evaluation measures that capture this? If not, I would stick to the YAGNI principle...

@ejhumphrey
Copy link
Collaborator

re: double octave, how do we feel about introducing an argument that determines the length of semitone_bitmap? this could be specified as an integer (12, 24, etc), which would allow arbitrary length semitone encodings ... I'm currently unsure of the value of this flexibility, but I could see it being handy. Alternatively, it could be a binary flag to indicate single / double octaves?

re: default value for pitch quality reduction ... these additional scale degrees need to be ignored for most scoring algorithms (maj/min, thirds, sevenths, etc), because the bit-vector represents semitones, and not pitch classes. To illustrate, sevenths(C:9, C:7) = 1, with the non-zero semitones being [0, 4, 7, 10, 14] vs [0, 4, 7, 10], and are equal when the 14 is ignored; but sevenths(C:7(2), C:7) = 0, with the non-zero semitones being [0, 2, 4, 7, 10] vs [0, 4, 7, 10], because the D now conflicts in the comparison.

IIRC, the only metric that looks at pitch class intersection is MIREX, in which case a quality bitmap is required, and a double-octave doesn't make much sense (the upper half could be empty?).

aren't chords just the most fun? 😀

@fdlm
Copy link
Contributor Author

fdlm commented Jul 5, 2017

This conflicts, however, with the MIREX evaluation, where indeed sevenths(C:7(2), C:7) = 1, because both C:9 and C:7(2) get mapped to C:7 (see the mapping rules here). I just confirmed that by running MusOOEvaluator.

Which brings us back to the philosophical question whether C:7(2) should be evaluated differently from C:9 ... 😀

@ejhumphrey
Copy link
Collaborator

To be fair, I'm not sure we know what MIREX does for something like C:7(2) (at least I don't with any confidence). The most recent MIREX vocabulary heuristics claim to be the following:

  1. Chord root note only;
  2. Major and minor: {N, maj, min};
  3. Seventh chords: {N, maj, min, maj7, min7, 7};
  4. Major and minor with inversions: {N, maj, min, maj/3, min/b3, maj/5, min/5}; or
  5. Seventh chords with inversions: {N, maj, min, maj7, min7, 7, maj/3, min/b3, maj7/3, min7/b3, 7/3, maj/5, min/5, maj7/5, min7/5, 7/5, maj7/7, min7/b7, 7/b7}.

The description references ignoring scale degrees beyond the first octave (the example they give is mapping G:7(#9) --> G:7 for the sevenths vocab), but says nothing about how to handle parenthetically added scale degrees. That said, this is simpler than how things used to be, so I guess that's something.

The closest thing I can find to the actual NEMA source that MIREX might be using is here but that repo is old. Is MusOOEvaluator being used for MIREX? Or does anyone have an idea of what is?

Since we can't seem to avoid chord philosophy and music theory (no surprises there haha), I think this whole mess stems from the practice of "bonus" scale degrees, and I think they should be done away with full stop. So, to your question, I don't think C:7(2) is a valid chord in general. No algorithm should produce it (or things like it), and annotators should be reprimanded (or worse) for writing chords like this (or C:maj(*3), or...). Therefore, as only reference annotations should contain such labels (with hopefully negligible frequency), these curious instances should be discarded from evaluation.

<rant> And, while we're at it, I'd argue this is because "chord estimation" as a task isn't strongly defined. You'll get very different annotations if one is prompted with "perform a functional analysis of this piece" versus "what guitar chords would I play for this song?" </rant>

@bmcfee
Copy link
Collaborator

bmcfee commented Jul 5, 2017

A bit more in favor of double-octave representations:

  • It's true that no current metric captures the distinction between C:9 and C:7(2), and yeah, the latter is kind of a garbage chord. But it might be useful to have the ability to capture extensions in the future -- I'm all about making new metrics if our current ones are broken. 😁
  • OTOH, if you're comparing a C:9 to C:sus2 (on, say, the mirex metric), I think it's reasonable to treat the 2 and the 9 as distinct, and not count them as a pitch class hit. Then again, I'm not a music theorist, so feel free to shoot me down on that.
  • Having an explicit representation for extensions would make it easier to invert encodings back to chord labels.
  • There's no harm in implementing it and leaving it disabled by default, or implementing the metrics so that they only look at the first twelve dimensions unless you set an extension flag to True.

@ejhumphrey
Copy link
Collaborator

ejhumphrey commented Jul 6, 2017

But it might be useful to have the ability to capture extensions in the future -- I'm all about making new metrics if our current ones are broken. 😁

This isn't an issue of metrics so much as it is problem definition. I'm offering the argument that perhaps extensions / bonus scale degrees aren't even chords, e.g. C:7(2) isn't a real thing. Now, if what that means to say is that both C:sus2 or C:7 are both acceptable chords for a given interval, that's one thing; but the problem definition should be careful to draw the line between pitch recognition and chord estimation.

To your point about comparing C:9 vs C:sus2, this again goes back to the goals of the system; it's not difficult to image that, in terms of guitar chords, C:sus2 makes more sense in context, e.g. hand position on the neck, fingerings of adjacent chords, than C:9, even though the former is missing the third and flat-7th, and the D is in the wrong octave. Or vice versa for piano. And IIRC, neither make a ton sense in terms of diatonic function (it'd be C:7).

@fdlm
Copy link
Contributor Author

fdlm commented Jul 7, 2017

In my understanding, the task of chord recognition should not consider playing techniques or practices for any particular instrument. The problem is, then, that some (many?) annotations out there are probably based on how a song is played on the guitar. This is why I argued before that maybe we cannot really distinguish between in which octave a certain pitch class is present as part of the harmony. I think this is also in line with the argument that chord recognition is different from pitch recognition. But I also think that this might not be the right place for such a discussion 😄

@ejhumphrey
Copy link
Collaborator

But I also think that this might not be the right place for such a discussion 😄

Agreed! but it's been fun / productive regardless. Any thoughts on where it'd be worth moving the conversation?

@fdlm
Copy link
Contributor Author

fdlm commented Jul 11, 2017

Not sure. There was an e-mail going around regarding an MIR slack channel, but the link does not seem to be active anymore. Anyways, we would need to define specific goals of such a discussion in order to make it meaningful, and include a broad community of people so that possible outcomes can be adopted widely.

@bmcfee
Copy link
Collaborator

bmcfee commented Aug 23, 2017

Is this closed by merging #252 ?

@ejhumphrey
Copy link
Collaborator

i do believe so, but would default to @fdlm

@fdlm
Copy link
Contributor Author

fdlm commented Aug 23, 2017

Yes, I think this is fixed now, thanks! 👍

@bmcfee bmcfee closed this as completed Aug 23, 2017
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

No branches or pull requests

3 participants