search

craffel / mir_eval

Evaluation functions for music/audio information retrieval/signal processing algorithms.
MIT License
588 stars 109 forks source link

[WIP] Implement current MIREX key scoring method and warn if using old one #339

Open stefan-baumann opened 2 years ago

stefan-baumann commented 2 years ago

This PR aims to solve the issue discussed in #337 in a backwards-compatible manner. An additional argument allow_descending_fifths was added to the key.weighted_score and key.evaluate methods, which is set to False by default (and thus being backwards-consistent), but raises a warning stating that this method is no longer used for MIREX. If set to True, descending fifth errors are given a score of 0.5, matching current MIREX scoring.

I'd love to get feedback on whether the maintainers approve of the way I implemented this change and whether they might have ideas how to improve keyword naming or the added documentation.

If the feedback is positive, I'll extend the tests to cover this behaviour as well.

craffel commented 2 years ago

@bmcfee @jpauwels @iansimon PTAL

stefan-baumann commented 2 years ago

but we should plan to switch the default to True eventually if it matches the new convention

In that case, we should probably change the warning message to reflect that. Maybe something like this?

The selected key scoring method does not match that currently used by MIREX. To use the same method, specify allow_descending_fifths=True. The default behaviour will change to allow_descending_fifths=True in the future.

Potentially substituting in the future with a specific version number.

craffel commented 2 years ago

but we should plan to switch the default to True eventually if it matches the new convention

In that case, we should probably change the warning message to reflect that. Maybe something like this?

The selected key scoring method does not match that currently used by MIREX. To use the same method, specify allow_descending_fifths=True. The default behaviour will change to allow_descending_fifths=True in the future.

Potentially substituting in the future with a specific version number.

SGTM

stefan-baumann commented 2 years ago

Okay, I added the note to the warning and the docstring, specifying that the default behaviour will change in the future.