Skip to content

Only "proper" chord types in mir_eval.chord.QUALITIES#397

Merged
bmcfee merged 8 commits intomir-evaluation:mainfrom
maximoskp:propertypes
Jan 23, 2025
Merged

Only "proper" chord types in mir_eval.chord.QUALITIES#397
bmcfee merged 8 commits intomir-evaluation:mainfrom
maximoskp:propertypes

Conversation

@maximoskp
Copy link
Copy Markdown
Contributor

Following the discussion in #396 and thanks to @bmcfee insightful comments, this PR simply removes some chord qualities in mir_eval.chord.QUALITIES that are "unreachable" by the regexp in the same file.

I.e., even though there the aforementioned QUALITITES dictionary includes keys for the b9, #9, #11 and b13 chord qualities, these are "unreachable" from the regexp. This is for a good reason, since (agreeing with @bmcfee) writing C:#9 is a bit odd - denoting it as C:7(#9) seems to be more proper.

Even though this PR doesn't fix any error per se, it resolves an edge-case error that occurs if, for instance, someone wants to iterate through mir_eval.chord.QUALITIES to get all possible chord qualities in mir_eval. Indeed, trying to mir_eval.chord.encode(C:#9) produces and error, but this error is expected to appear if, for some reason, someone tries to iterate through all qualities.

This PR simply removes the four qualities that result in this errors in this edge case. Disclaimer: I haven't tested whether this change breaks some other functionality, but I believe it shouldn't, since any other such functionality would ultimately have to be related with the regexp that properly checks for nicely-formed qualities.

Thank you @bmcfee for the help and the clarifications!

@bmcfee
Copy link
Copy Markdown
Collaborator

bmcfee commented Jan 15, 2025

Thanks @maximoskp , and apologies for the slow response on this!

The core change looks good to me. I've added a couple of comments above on what appear to be unnecessary changes in the tests. Once that's sorted, I'm happy to merge.

Comment thread test_chord_types.ipynb Outdated
Comment thread tests/test_chord.py Outdated
Comment thread tests/test_chord.py Outdated
Comment thread tests/test_chord.py Outdated
Comment thread tests/test_chord.py Outdated
Comment thread tests/test_chord.py Outdated
@bmcfee bmcfee merged commit 13909a6 into mir-evaluation:main Jan 23, 2025
@bmcfee bmcfee added this to the 0.8 milestone Feb 6, 2025
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

Successfully merging this pull request may close these issues.

2 participants