Allchordtypes

[maximoskp]

Hi! I've noticed that not all chord qualities in

mir_eval/chord.py line 245

were understood by chord.encode.

For example, there was an error when calling:

c = mir_eval.chord.encode('C:#11')

The fix was to add the missing chord qualities (I think they were 3) in the "monster" regext in line 347. So the change is only there.

I did some test on my own but couldn't properly integrate it in the official test/test_chord.py file, so I simply left a comment on how I did my tests there.

That's my first pull request on another repository on github so I'll be happy follow any instructions to make it more proper / professional. Thanks!

[craffel]

@ejhumphrey can you take a look? I still know nothing about chords.

[bmcfee]

Jumping in here, since I think @ejhumphrey may be permanently out of the game on this stuff, and I think that regex is my fault anyway. 🤣

High level, I don't think the proposed change is consistent with Harte's grammar, as described in https://qmro.qmul.ac.uk/xmlui/bitstream/handle/123456789/534/HARTETowardsAutomatic2010.pdf?sequence=1 chapter 4 (starting around page 86) and summarize in BNF form on page 105:

<chord> ::= <pitchname> ":" <shorthand> ["("<ilist>")"]["/"<interval>]
| <pitchname> ":" "("<ilist>")" ["/"<interval>]
| <pitchname> ["/"<interval>]
| "N"
<pitchname> ::= <natural> | <pitchname> <modifier>
<natural> ::= "A" | "B" | "C" | "D" | "E" | "F" | "G"
<modifier> ::= "b" | "#"
<ilist> ::= ["*"] <interval> ["," <ilist>]
<interval> ::= <degree> | <modifier> <interval>
<degree> ::= <digit> | <digit> <degree> | <degree> "0"
<digit> ::= "1" | "2" | "3" | "4" | "5" | "6" | "7" | "8" | "9"
<shorthand> ::= "maj" | "min" | "dim" | "aug" | "maj7" | "min7" | "7"
| "dim7" | "hdim7" | "minmaj7" | "maj6" | "min6" | "9"
| "maj9" | "min9" | "sus2" | "sus4"

In JAMS we extended the definition a little to fill in some obvious gaps in supported qualities(ie missing 11 and 13), but the basic structure of the grammar is unchanged. (Note: the grammar above can be standardized into a fully right-recursive form, and therefore collapses down to a regular expression, which is how I generated the monster regex in the first place.)

As I understand it, the notation there allows accidentals on notes or intervals, but not qualities ("shorthand" in the above). If you want to raise or flatten a particular note within a chord, I suppose you would do it by something like C:7(#9).

This isn't to say we can't change how the grammar is defined, but at present we have a reasonably functional implementation that can parse strings that validate under the current grammar and do the "right" things with, eg, identifying pitch classes, roots, and bass notes. Changing the grammar would necessitate changing the downstream parsing as well, and I'd prefer to avoid that unless we have a strong reason to. "Strong reason" would be, eg, failing to handle existing and widely used annotations, or inability to represent certain common qualities.

[bmcfee]

I was thinking on this a bit more, and I suppose the reasoning behind not allowing accidentals on chord qualities is that it could lead to some ambiguities. G#:9 and G:#9 would mean two entirely different things, and while the grammar we use requires a : to separate the root from the quality, it still seems brittle to me. (Especially if some datasets require automated reformatting to separate root from quality, eg they use G7 instead of G:7.)

AFAIK, G:7(#9) is much clearer and more commonly used for exactly this reason. But if I have this wrong, someone who actually plays music should chime in. 😁

[maximoskp]

Hi! Thank you @bmcfee for the detailed explanations and ideas!

This pull request simply aims to make use of all the qualities that are listed in mir_eval.chord.QUALITIES. If I'm not mistaken, in the current code some QUALITIES are redundant / unnecessary. For instance, the quality corresponding to the #9 dictionary key will never be reached because the cool regex will not allow it. So, even though this causes no error if you don't use it, it is confusing (or at least was for me) that this key of the QUALITIES dictionary should be there doing nothing...

So, for me and if I'm correct, the options are to either modify the regex or remove the mir_eval.chord.QUALITIES entries that are unreachable. Of course, I might have misunderstood what this QUALITIES dictionary is meant to indicate in the grand scheme of things (it's possibly used in other ways somewhere else in this diverse and versatile package), so I might be absolutely mistaken...

I understand that C:7(#9) makes more sense in several ways, however, I found it super convenient that I could iterate through all the QUALITIES that mir_eval could understand and use it as a lexicon of all possible chords types in another thing that I'm doing. The tradeoff was to have chord annotations like C:#9, which are a bit odd, but for my application that did the trick.

I understand the broader scope though, and I will be happy to cancel the pull request or have it rejected - I don't know what is the preferable way - please let me know if I need to do something and I will be happy to do it :)

[bmcfee]

This pull request simply aims to make use of all the qualities that are listed in mir_eval.chord.QUALITIES

Ah, that's an excellent point: you're correct that the validator blocks some of the qualities that are included in the QUALITIES dict. I suppose this came about because the qualities dict and the regex were developed independently and later merged. The merger happened because the old validator (originally developed with the QUALITIES dict, which I think can be traced back to Tae Min Cho's dissertation work circa 2012) was slow and not entirely correct. "Correct" is a little squishy here because I don't think there was a formally defined grammar in use at that point. We later brought in the regex as a way to codify what strings are and are not supported as chord labels.

I found it super convenient that I could iterate through all the QUALITIES that mir_eval could understand and use it as a lexicon of all possible chords types in another thing that I'm doing.

Yeah, that's a good (if not exactly supported) use case. I've done similar things elsewhere.

So, for me and if I'm correct, the options are to either modify the regex or remove the mir_eval.chord.QUALITIES entries that are unreachable.

In this case, I think I would opt for removing the unreachable qualities, given the potential ambiguity noted above. Since there are no reachable code paths that depend on them, I don't think this will result in any behavior change.

This is not to say that we shouldn't ever expand the qualities and/or regex, in particular with extended chords. There is actually a separate issue #274 and PR #275 that has to do with improving support for above-octave extensions, and I think we can do a lot more with that, eg by allowing a two-octave bitmapping (so that 9 and 2 can be kept distinct if needed, etc). But that's getting a bit off topic.